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.

Sign up for GitHub

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

Jump to bottom

DependencyClassMethodDecorator: Prevent duplication of arguments #2643

Merged
merged 1 commit into from Jul 9, 2022
Merged

DependencyClassMethodDecorator: Prevent duplication of arguments #2643

merged 1 commit into from Jul 9, 2022

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jul 9, 2022

@jtojnar jtojnar mentioned this pull request Jul 9, 2022
Closed
@jtojnar jtojnar force-pushed the fix-duplicate-constructor-deps branch from 576c4cc to f9ddb0b Compare July 9, 2022 03:49
@TomasVotruba
Copy link
Member

Thank you 👍

We'll need a test fixture that cover this.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 9, 2022

Do you have an example of a test I can use as a template? Or would covering it using an ad-hoc rule as in https://getrector.org/demo/2b5d2da3-f97e-48aa-ab52-97268718bb40 be okay?

@TomasVotruba
Copy link
Member

TomasVotruba commented Jul 9, 2022 •
edited

You can add it to https://github.com/rectorphp/rector-src/blob/main/rules-tests/DependencyInjection/Rector/ClassMethod/AddMethodParentCallRector/AddMethodParentCallRectorTest.php → /Fixture directory. There you'll find other tested cases

@jtojnar jtojnar force-pushed the fix-duplicate-constructor-deps branch from f9ddb0b to 483074b Compare July 9, 2022 15:11
@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 9, 2022

AddMethodParentCallRector does not look like uses DependencyClassMethodDecorator. But I looked for DependencyClassMethodDecorator and it is only used by ClassDependencyManipulator, which is solely used by PropertyAddingPostRector. And that in turn uses PropertyToAddCollector so I checked its uses and NewToConstructorInjectionRector fits the use case.

@jtojnar
DependencyClassMethodDecorator: Prevent duplication of arguments
20c848f 
When a parent class has a constructor-injected dependency and `ClassDependencyManipulator`
tries to add the same dependency to the constructor of the child class,
the child constructor would have two parameters with the same name.

Let’s skip the arguments whose all characteristics (name, type, byRefness, variadicity)
match one of the existing arguments of the child constructor.

In the future, we might also want to handle duplication of parameters
with non-matching characteristics by renaming the parameters.

Fixes: rectorphp/rector#7286
@jtojnar jtojnar force-pushed the fix-duplicate-constructor-deps branch from 483074b to 20c848f Compare July 9, 2022 15:27
@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 9, 2022

Seems to pass now.

@TomasVotruba
Copy link
Member

Thank you 👏

@TomasVotruba TomasVotruba merged commit d3cb26f into rectorphp:main Jul 9, 2022
@jtojnar jtojnar deleted the fix-duplicate-constructor-deps branch July 9, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsonasik samsonasik samsonasik approved these changes

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jtojnar @TomasVotruba @samsonasik