Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Added closure service factories#12115
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f2cba65 to1725733Comparestof commentedOct 4, 2014
I don't like the idea of the Pimple-like closures. It makes the behavior of the factory callable inconsistent between the case of a closure and the case of other type of callables. Thus, using such closures will require marking the dependencies as public (as they are retrieved dynamically), and prevents the ContainerBuilder to perform any optimization on the service instantiation (as it is inside the closure). So using such Pimple-like closures has lots of drawbacks. |
8af5df4 to73ed6fcCompareunkind commentedOct 4, 2014
Pimple-like syntax has been removed. |
73ed6fc to3a9e4f8CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we don't make classes final in Symfony generally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's bad practice to be honest. No reason to inherit this class, you can make decorator to extend behaviour.
bff5a80 to92b59f2CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
add a blank line beforereturn
92b59f2 tob6067ffCompareunkind commentedOct 5, 2014
fabpot commentedOct 5, 2014
No, I still have too many questions/doubts to be able to merge this in a hurry, that's never a good idea. |
unkind commentedOct 11, 2014
The biggest problem of this PR is a coupling of |
fabpot commentedFeb 5, 2015
I think I'm 👎 on this one. It adds a lot of complexity for very rare use cases that you can do another way. |
unkind commentedFeb 5, 2015
I have to remind that initially it was done for the Some use cases:
If you say that it's a "very rare case", you have to admit that you either don't want array file configs at all or expression language factory is a "very rare case" as well. But it won't be merged in this way of course, because |
stof commentedFeb 5, 2015
@unkind in your specific case, I would rather define services for your database connection (using your singleton getter as factory) and then inject the service in your The issue when introducing the dumping of closures is that we add lots of complexity for that, and that we only allow dumping a subset of closures (anonymous functions which don't depend on their context at all) |
unkind commentedFeb 5, 2015
|
stof commentedFeb 5, 2015
sure we can throw an exception in this case. But it means we don't support them, meaning we only support a subset of closures, not all closures. |
unkind commentedFeb 5, 2015
Partial support is better than nothing in this case IMO. |
This PR adds closure service factories for the
DependencyInjectioncomponent. There are 2 kinds of closures:It would be useful for something like
ArrayFileLoaderdescribed in#11953.