-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DependencyInjection] Added closure service factories #12115
Conversation
f2cba65
to
1725733
Compare
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
to
73ed6fc
Compare
Pimple-like syntax has been removed. |
73ed6fc
to
3a9e4f8
Compare
/** | ||
* @author Nikita Konstantinov <unk91nd@gmail.com> | ||
*/ | ||
final class SuperClosureDumper implements ClosureDumperInterface |
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.
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
to
92b59f2
Compare
return $this->services['bar'] = call_user_func(function (\stdClass $foo) { | ||
$bar = clone $foo; | ||
$bar->bar = 'bar'; | ||
return $bar; |
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.
add a blank line before return
92b59f2
to
b6067ff
Compare
No, I still have too many questions/doubts to be able to merge this in a hurry, that's never a good idea. |
The biggest problem of this PR is a coupling of |
I think I'm 👎 on this one. It adds a lot of complexity for very rare use cases that you can do another way. |
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 |
@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) |
|
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. |
Partial support is better than nothing in this case IMO. |
This PR adds closure service factories for the
DependencyInjection
component. There are 2 kinds of closures:It would be useful for something like
ArrayFileLoader
described in #11953.