-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
do not add redundant method calls from interface injection #541
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
do not add redundant method calls from interface injection #541
Conversation
… already been set manually
|
re: #535 (comment) @vicb: i am sure there could be some use cases, maybe we should just disable interface injection entirely if a manual call has been set? |
|
I don't think we have any basis to determine whether a user wants a method to be called multiple times or only once. If this is an issue we should remove any interface injectors from the core. |
|
@kriswallsmith: well similar issues can happen with any 3rd party bundle that uses interface injection. so i think the approach of just disabling interface injection if a single manual method call is set, should work best here: aka setting a manual method calls means you want to manually manage setter injection. should make things clear in the docs too. |
|
Symfony is built around the user being explicit about his intention and us not making any assumptions. This patch bakes in an assumption that the user only wants a method to be called once and creates an unnecessary limitation. |
|
I have reverted this patch for now as I think @kriswallsmith and @schmittjoh have good arguments. |
|
I agree. Interfaces and parent definitions are “upstream” and should be prepended.
|
|
Well then you still have redundant method calls, which may or may not lead to simply adding overhead or actually causing issues if the second method call doesnt simply override the previous one. So I still think the best solution is to simply disable interface injection for any definition that defines an explicit method call. |
|
I have opened a pull that implements what I suggested: #544 |
|
closing this pull .. |
do not add method calls from interface injection if a method call has already been set manually
fixes issue caused by #535