10000 do not add redundant method calls from interface injection by lsmith77 · Pull Request #541 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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.

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
Merged
merged 1 commit into from
Apr 13, 2011
Merged

do not add redundant method calls from interface injection #541

merged 1 commit into from
Apr 13, 2011

Conversation

lsmith77
Copy link
Contributor

do not add method calls from interface injection if a method call has already been set manually

fixes issue caused by #535

@lsmith77
Copy link
Contributor Author

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?

@fabpot fabpot merged commit c9be18a into symfony:master Apr 13, 2011
@kriswallsmith
Copy link
Contributor

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.

@lsmith77
Copy link
Contributor Author

@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.

@kriswallsmith
Copy link
Contributor

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.

@fabpot
Copy link
Member
fabpot commented Apr 13, 2011

I have reverted this patch for now as I think @kriswallsmith and @schmittjoh have good arguments.

@fabpot fabpot reopened this Apr 13, 2011
@kriswallsmith
Copy link
Contributor

I agree. Interfaces and parent definitions are “upstream” and should be prepended.
On Wednesday, April 13, 2011 at 5:38 AM, schmittjoh wrote:

As stated on the other PR, I think prepending the interface injection calls instead of appending them is the best option. I also choose that when you inherit from another definition that has method calls, and IIRC this is also what Spring does in these cases.

Thoughts?

Reply to this email directly or view it on GitHub:
#541 (comment)

@lsmith77
Copy link
Contributor Author

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.

@lsmith77
Copy link
Contributor Author

I have opened a pull that implements what I suggested: #544

@lsmith77
Copy link
Contributor Author

closing this pull ..
(btw: i think this pull was also additionally flawed because it would probably break if one would use multiple interfaces which are configured to trigger injections)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lsmith77 @kriswallsmith @fabpot
0