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] Automatically detect the definitions class when possible#19191
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
GuilhemN commentedJun 27, 2016
| Q | A |
|---|---|
| Branch? | "master" |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19161 |
| License | MIT |
| Doc PR |
| $returnType = $reflectionClass->getReturnType(); | ||
| } | ||
| if (null !== $returnType && !$returnType->isBuiltin()) { |
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.
It looks like hhvm doesn't detect built in types... Maybe i should just remove this check as this is unexpected anyway ?
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.
should be reported as a bug to HHVM then
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.
OK no, the issue is that the version of HHVM we use does not support scalar type hints, and so considers it as a typehint for the classSymfony\Component\DependencyInjection\Tests\Fixtures\int (which is indeed not builtin).
So please simply skip this test on HHVM
4207797 to8eb1972Compare| } | ||
| if (is_string($factory) && function_exists($factory)) { | ||
| $reflectionFunction = new \ReflectionFunction($factory); |
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.
Why using atry/catch block for\ReflectionMethod but not here?
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.
this code will not work fine for'MyClass::staticMethod' callables, which also belong to a class and so may need to resolveself andparent
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.
08c6162 to3c379b3Compare| } | ||
| try { | ||
| $reflectionFunction = new \ReflectionMethod($class, $factory[1]); |
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.
For historical reasons, reflection class vars are named$r and method$m in the code base.
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.
ok, updated :)
| } | ||
| } else { | ||
| if ($factory[0] instanceof Reference) { | ||
| $previous[$id] = ''; |
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 generally usetrue rather than an empty string
| } | ||
| $factory = $definition->getFactory(); | ||
| if (null === $factory || $definition->getClass()) { |
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.
null !== $definition->getClass()
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.
good catch, thanks !
GuilhemN commentedJul 6, 2016
Do you see anything else to change ? |
| (string) $factory[0], | ||
| $factoryDefinition = $container->findDefinition((string) $factory[0]), | ||
| $previous | ||
| ); |
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.
should be on one line.
GuilhemN commentedJul 11, 2016
updated@fabpot |
bee5364 toc044df0Comparedunglas commentedSep 6, 2016
👍 |
| $class = null; | ||
| if (is_string($factory)) { | ||
| try { | ||
| $m = new \ReflectionFunction($factory); |
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 don't like having reflection involved in a compiler pass. I know that this is the case for the autowiring pass (and one of the reasons I don't like it), but I'm not comfortable adding this "constraint" for the default and mainstream behavior of the DI engine.
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.
That's only used if the user doesn't explicitly define the service class (so will never be executed on current code base as it is currently throwing an error).
I see only static analysis to get ride of reflection but i don't think that's better.
As an example, this pass could be used to create services from callables (that's what psr is doing if i'm not wrong?) used with autowiring:
class ServiceBuilder {publicfunctiongetFoo():Foo {}publicfunctiongetBar(Foo$foo):Bar {}publicfunctiongetDeclaredServices() {return ['foo','bar']; }}
xabbuh commentedSep 19, 2016
I can see that this compiler pass makes the configuration of services a bit easier and less repetitive when using return types with PHP 7. To me it's fine to do the calls to the reflection API just during compile time and only when the classes haven't been configured explicitly. So I am 👍 on this feature. |
dunglas commentedSep 19, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I also think that it's a nice DX improvement. And it should not hurt as it is an opt-in feature. 👍 |
fabpot commentedSep 19, 2016
Thank you @Ener-Getick. |
…ions class when possible (Ener-Getick)This PR was merged into the 3.2-dev branch.Discussion----------[DependencyInjection] Automatically detect the definitions class when possible| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19161| License | MIT| Doc PR |> Thanks to the features of php 7.0 we can now guess the class of a service created with a factory:> ```php> function myFactory(): MyServiceClass> {> }> ```>> So I propose to create a new pass to automatically update the services definition when possible. This is particularly useful for autowiring (this way you don't have to copy-paste the class name of the service, especially when this is from a third party library).>> What do you think ?Commits-------63afe3c [DependencyInjection] Automatically detect the definitions class when possible
…n PassConfig (hason)This PR was merged into the 3.2-dev branch.Discussion----------[DependencyInjection] Fix FactoryReturnTypePass position in PassConfig| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19161,#19191| License | MIT| Doc PR |Commits-------dfb5cc3 [DependencyInjection] Fix FactoryReturnTypePass position in PassConfig
nicolas-grekas commentedJan 2, 2017
GuilhemN commentedJan 2, 2017
@nicolas-grekas as discussed on slack, i agree we should deprecate this in favor of#20264. |
…-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Optional class for named services| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Continues#20264:- makes the id-to-class mapping context-free (no `class_exist` check),- deals with ChildDefinition (which should not have this rule applied on them),- deprecates FactoryReturnTypePass as discussed on slack and reported in this comment:#19191 (comment)From@hason:> I prefer class named services (in applications) because naming things are too hard:``` yamlservices: Vendor\Namespace\Class: class: Vendor\Namespace\Class autowire: true```> This PR solves redundant parameter for class:``` yamlservices: Vendor\Namespace\Class: autowire: true```> Inspirations:https://laravel.com/docs/5.3/container,#18268,http://php-di.org/,Commits-------a18c4b6 [DI] Add tests for class named services71b17c7 [DI] Optional class for named services