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

[ErrorNamesPropertyToConstantRector] add new rule to fix #280 #372

Merged

Conversation

JohJohan
Copy link
Contributor

No description provided.

@johanadivare
Copy link
Contributor

Im not sure what the best way to find if property $errorNames is used. I saw we have one rector rule (JMSInjectPropertyToConstructorInjectionRector) to change properties but dont think that is usefull for this case see the replace fixture for example.

*/
public function getNodeTypes(): array
{
return [ClassConstFetch::class];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be PhpParser\Node\Expr\StaticPropertyFetch?

@stefantalen
Copy link
Contributor

I'd use Rector\Core\Reflection\ReflectionResolver to check if $node->class is a subclass of Symfony\Component\Validator\Constraint 🙂

@JohJohan JohJohan force-pushed the ErrorNamesPropertyToConstantRector branch from 361ebb8 to 09a1bec Compare February 27, 2023 10:06
@johanadivare
Copy link
Contributor

I'd use Rector\Core\Reflection\ReflectionResolver to check if $node->class is a subclass of Symfony\Component\Validator\Constraint slightly_smiling_face

@stefantalen thanks that was very helpful

@JohJohan JohJohan force-pushed the ErrorNamesPropertyToConstantRector branch from 09a1bec to 4fa223c Compare February 27, 2023 10:11
@johanadivare
Copy link
Contributor

I dont know why https://github.com/rectorphp/rector-symfony/actions/runs/4281227323/jobs/7453990047 is failing. I did composer update and ran vendor/bin/rector --clear-cache so rector shouldn't be failing.

On the other hand the rector always fails with this message: Error: '{brancName}' branch not found. it would be nice to see the actual rector output is that possible? In this case i would then see what is wrong.

@stefantalen
Copy link
Contributor

Try composer fix-cs 😉 (If you look at the action, the ECS check is failing because it wants to change your code)

@JohJohan JohJohan force-pushed the ErrorNamesPropertyToConstantRector branch from 4fa223c to 546ad93 Compare February 27, 2023 11:29
@johanadivare
Copy link
Contributor

Try composer fix-cs wink (If you look at the action, the ECS check is failing because it wants to change your code)

Yes thank you :) i now see i can also open the logs above.

@johanadivare
Copy link
Contributor

@samsonasik can you do a cr?

@TomasVotruba
Copy link
Member

We'll need a rebase here.

if (! $classReflection->isSubclassOf('Symfony\Component\Validator\Constraint')) {
return null;
}
if ($this->nodeNameResolver->getName($node->name) !== 'errorNames') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($this->nodeNameResolver->getName($node->name) !== 'errorNames') {
if (! $this->nodeNameResolver->isName($node->name, 'errorNames')) {

Copy link
Member

@TomasVotruba TomasVotruba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's look very good!
Please rebase, fix one detail and ready to merge 👍

$parametersAcceptor = ParametersAcceptorSelector::selectSingle($extendedMethodReflection->getVariants());
$parametersAcceptorWithPhpDocs = ParametersAcceptorSelector::selectSingle(
$extendedMethodReflection->getVariants()
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use:

ParametersAcceptorSelector::combineAcceptors(
$extendedMethodReflection->getVariants())

That ensure multi variants is supported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@JohJohan JohJohan force-pushed the ErrorNamesPropertyToConstantRector branch from 546ad93 to 296d381 Compare March 30, 2023 06:20
@JohJohan JohJohan force-pushed the ErrorNamesPropertyToConstantRector branch from 296d381 to 8255805 Compare March 30, 2023 06:25
@johanadivare
Copy link
Contributor

@TomasVotruba and @samsonasik made the requested changes and pipeline passes can you cr again

@TomasVotruba
Copy link
Member

Looks good to me 👍

@samsonasik Feel free to merge when ready for you 🙏

@samsonasik samsonasik merged commit 17031bf into rectorphp:main Mar 30, 2023
@samsonasik
Copy link
Member

Thank you @johanadivare

@JohJohan JohJohan deleted the ErrorNamesPropertyToConstantRector branch March 30, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants