-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Add PHP service factories #23935
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
Conversation
Considering there will be support of fluent PHP configuration (#23834), I'd rather put effort to create closure-based factories, e.g. <?php
di\services()
->instanceof(Bar::class)
->factory(function () {
return new Bar(new Foo());
}) I tried to bring this feature in the past (#12115), but it was rejected because "It adds a lot of complexity for very rare use cases that you can do another way". However, with fluent PHP configuration in the core it wouldn't be "rare case" anymore I guess. Even if one prefers YAML/XML formats, he still can create fluent PHP config file with the factories instead of "service factory" which you propose. |
@unkind nice.. didnt even thought of such an approach :) Yet we cannot dump such a closure factory. So im not sure it works out; the current approach mostly leverages existing features. For the config part is was planning to have some I like the fact we can create powerful services with a minimal config. |
@unkind Closure-based factories are incompatible with the dumping of an optimized container, as we cannot dump the closure (due to a closure being able to reference variables from its defining scope). And dumping the final container to a PHP class is necessary (at least if you care about performance). Btw, I see a big drawback with this approach: any time your factory references another of its methods to get a dependency, it will create its own instance of the services, which will not be the same than the shared service used by the container. This can lead to hard-to-debug behaviors (overwriting the |
Fair point. We could set shared to false by default, but it really depends what the user does. I.e. if it mimics sharing/memoization in PHP. We dont know that... I tend not to care about using non-shared |
Don't reference such variables. There is no need in it. |
@unkind even then, getting the source code of a closure to be able to replicate it in the dumped source code cannot be done in a clean way (and is impossible if you have multiple closures defined on the same line, as closures only have a start and end line in reflection, like other functions, and they don't have a name to resolve ambiguities when having multiple closures on the line). |
There are third party tools which do it quite well.
I know these restrictions, but they are too artificial and can be detected at container compilation time. Likewise, I can say that Symfony doesn't have full support of YAML, but it seems like it isn't a disaster. |
Agree with @nicolas-grekas, this is not about proposed Parsing |
In fact, this PR made me realize we have a "bug" with autowiring (that we should fix as a feature in 3.4): |
@nicolas-grekas dont we already do that.. looking at |
oh indeed! |
return $methodsToFactorize; | ||
} | ||
|
||
private function getClass(\ReflectionMethod $reflectionMethod) |
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.
you should be able to use Symfony\Component\DependencyInjection\LazyProxy\ProxyHelper::getTypeHint()
instead
Closing after some discussion with @nicolas-grekas This PR can go many ways, and i want to play with #23834 first actually :) |
This is a POC that more or less implements a feature described in #23819.
It allows to automate service creation from a factory, which itself is a service. You tag it, and basically it scrapes methods to factorize as new services.
So far, no real annotations are involved. Im not sure a whole system is worth it for this feature solely. Yet it introduces one new annotation, in the spirit of
@required
, to indicate which methods to factorize;@service
or@service my.id
. This convention can also be used for #23898.To me this is convenient. Further configuration can be applied static (this PR is not that far though); adding tags, sharing, factory arguments etc. It requires no real alias support; as that would be simply forwarding methods in this context.
It checks return types for the class value, so it works on PHP7 only for now. I abused tests for this to demonstrate :) I wasnt planning for parsing
@return
so <PHP7 requires further configuration.Quick example;
This creates the services
some_foo
andsome.bar
.Thoughts?
/cc @GuilhemN