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

[FrameworkBundle] Let GetToConstructorInjectionRector work with trait as well #564

Merged
merged 9 commits into from
Aug 9, 2018
Merged

[FrameworkBundle] Let GetToConstructorInjectionRector work with trait as well #564

merged 9 commits into from
Aug 9, 2018

Conversation

TomasVotruba
Copy link
Member

Closes #560

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Aug 9, 2018 •
edited
Loading

@mssimi This should fix it. This is the parent trait test case: https://github.com/rectorphp/rector/pull/564/files#diff-e5bdcf0566e5d578d1857ed74d5fd27e

@TomasVotruba TomasVotruba mentioned this pull request Aug 9, 2018
Closed
@mssimi
Copy link
Contributor

mssimi commented Aug 9, 2018

Tested on example I posted, still does nothing.

@TomasVotruba
Copy link
Member Author

Thanks for feedback.

Got it 👍

image

@mssimi
Copy link
Contributor

mssimi commented Aug 9, 2018

Yep it works now:)

@mssimi
Copy link
Contributor

mssimi commented Aug 9, 2018

Actually Interface should be used there.

\Symfony\Bundle\FrameworkBundle\Translation\Translator

\Symfony\Bundle\FrameworkBundle\Translation\TranslatorInterface

@TomasVotruba
Copy link
Member Author

Could you add it via PR?
I'm not sure it's compatible or how to determine the proopper Interface to make it always work

@TomasVotruba TomasVotruba merged commit 2bbeffc into master Aug 9, 2018
@mssimi
Copy link
Contributor

mssimi commented Aug 9, 2018

Well, if you don't use Interface, exception is thrown.

@TomasVotruba TomasVotruba deleted the controller-trait branch August 9, 2018 12:03
@TomasVotruba
Copy link
Member Author

Luckily, it was an easy pick. Your application helped me to find it :)
#565

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mssimi mssimi mssimi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TomasVotruba @mssimi