Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Php81] Skip ReadOnlyPropertyRector on read only class #3236

Merged
merged 5 commits into from Dec 22, 2022

Conversation

samsonasik
Copy link
Member

@samsonasik
Copy link
Member Author

/cc @Wirone

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@Wirone
Copy link
Contributor

Wirone commented Dec 22, 2022

Currently rule only checks if readonly should be added to promoted properties, but that PR to Laminas had different scenario: promoted properties already had readonly.

There should be also fixture for scenario where class' promoted properties have readonly and class doesn't have it: when readonly is added on class level, properties' readonlys should be removed.

@samsonasik
Copy link
Member Author

samsonasik commented Dec 22, 2022

That already covered at ReadonlyClassRector, see this transformation

final class OnlyReadonlyProperty2
{
public function __construct(private readonly string $property)
{
}
}
?>
-----
<?php
namespace Rector\Tests\Php82\Rector\Class_\ReadOnlyClassRector\Fixture;
final readonly class OnlyReadonlyProperty2
{
public function __construct(private string $property)

the multi rules back to add readonly again on ReadonlyPropertyRector because found a readonly property, which was doesn't check the class already readonly so flip flop was happen ,this PR fix it :)

@Wirone
Copy link
Contributor

Wirone commented Dec 22, 2022

Aaahhh, ok, so it was removed and added again, hence no diff 😅 Thanks for clarification!

@TomasVotruba TomasVotruba merged commit ea22343 into main Dec 22, 2022
@TomasVotruba TomasVotruba deleted the skip-readonly-class-property branch December 22, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants