[DependencyInjection] Automatically detect the definitions class when possible #19191

GuilhemN · 2016-06-27T10:16:49Z

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:
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 ?

GuilhemN · 2016-06-27T10:36:09Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+            $returnType = $reflectionClass->getReturnType();
+        }
+
+        if (null !== $returnType && !$returnType->isBuiltin()) {


It looks like hhvm doesn't detect built in types... Maybe i should just remove this check as this is unexpected anyway ?

should be reported as a bug to HHVM then

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 class Symfony\Component\DependencyInjection\Tests\Fixtures\int (which is indeed not builtin).

So please simply skip this test on HHVM

dunglas · 2016-06-27T13:28:46Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+        }
+
+        if (is_string($factory) && function_exists($factory)) {
+            $reflectionFunction = new \ReflectionFunction($factory);


Why using a try/catch block for \ReflectionMethod but not here?

this code will not work fine for 'MyClass::staticMethod' callables, which also belong to a class and so may need to resolve self and parent

@dunglas good catch, updated.

@stof this is managed here.

fabpot · 2016-06-28T05:18:07Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+            }
+
+            try {
+                $reflectionFunction = new \ReflectionMethod($class, $factory[1]);


For historical reasons, reflection class vars are named $r and method $m in the code base.

ok, updated :)

stof · 2016-07-01T07:50:23Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+            }
+        } else {
+            if ($factory[0] instanceof Reference) {
+                $previous[$id] = '';


we generally use true rather than an empty string

xabbuh · 2016-07-05T07:53:06Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+        }
+
+        $factory = $definition->getFactory();
+        if (null === $factory || $definition->getClass()) {


null !== $definition->getClass()

good catch, thanks !

GuilhemN · 2016-07-06T18:21:55Z

Do you see anything else to change ?

fabpot · 2016-07-08T07:00:48Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+                    (string) $factory[0],
+                    $factoryDefinition = $container->findDefinition((string) $factory[0]),
+                    $previous
+                );


should be on one line.

GuilhemN · 2016-07-11T21:06:42Z

updated @fabpot

… possible

dunglas · 2016-09-06T21:31:18Z

👍

fabpot · 2016-09-14T22:19:16Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

+        $class = null;
+        if (is_string($factory)) {
+            try {
+                $m = new \ReflectionFunction($factory);


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.

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 { public function getFoo(): Foo {} public function getBar(Foo $foo): Bar {} public function getDeclaredServices() { return ['foo', 'bar']; } }

xabbuh · 2016-09-19T07:13:19Z

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 · 2016-09-19T08:49:11Z

I also think that it's a nice DX improvement. And it should not hurt as it is an opt-in feature. 👍

fabpot · 2016-09-19T19:26:15Z

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 · 2017-01-02T09:31:45Z

I'm looking at #20264 seriously, and I'm stumbling upon this one, which is colliding badly.
I think #20264 should win the competition, but we can't, because BC.
Thus, I'm going to propose to deprecate this behavior.

GuilhemN · 2017-01-02T11:37:20Z

@nicolas-grekas as discussed on slack, i agree we should deprecate this in favor of #20264.
#20264 can be used more often and is safer so it should win against factories resolution.

@hason

…-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: ``` yaml services: Vendor\Namespace\Class: class: Vendor\Namespace\Class autowire: true ``` > This PR solves redundant parameter for class: ``` yaml services: 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 services 71b17c7 [DI] Optional class for named services

carsonbot added Status: Needs Review DependencyInjection Feature labels Jun 27, 2016

GuilhemN reviewed Jun 27, 2016
View reviewed changes

GuilhemN force-pushed the FACTORIES branch 3 times, most recently from 4207797 to 8eb1972 Compare June 27, 2016 12:32

dunglas reviewed Jun 27, 2016
View reviewed changes

GuilhemN force-pushed the FACTORIES branch 3 times, most recently from 08c6162 to 3c379b3 Compare June 27, 2016 17:09

fabpot reviewed Jun 28, 2016
View reviewed changes

GuilhemN force-pushed the FACTORIES branch from 3c379b3 to ec1d5d9 Compare June 28, 2016 11:40

stof reviewed Jul 1, 2016
View reviewed changes

GuilhemN force-pushed the FACTORIES branch from ec1d5d9 to 76d3597 Compare July 1, 2016 11:09

xabbuh reviewed Jul 5, 2016
View reviewed changes

GuilhemN force-pushed the FACTORIES branch from 76d3597 to b5f7cd6 Compare July 5, 2016 15:28

fabpot reviewed Jul 8, 2016
View reviewed changes

GuilhemN force-pushed the FACTORIES branch from b5f7cd6 to 3f67f49 Compare July 11, 2016 21:05

GuilhemN force-pushed the FACTORIES branch 2 times, most recently from bee5364 to c044df0 Compare July 23, 2016 18:13

[DependencyInjection] Automatically detect the definitions class when…

63afe3c

… possible

GuilhemN force-pushed the FACTORIES branch from c044df0 to 63afe3c Compare July 31, 2016 10:30

GuilhemN mentioned this pull request Aug 10, 2016

[RFC] [DependencyInjection] Automatically detect definitions class based on their factory #19161

Closed

GuilhemN mentioned this pull request Aug 30, 2016

[HttpKernel] Added nullable support for php 7.1 and below #19785

Closed

fabpot reviewed Sep 14, 2016

View reviewed changes

fabpot merged commit 63afe3c into symfony:master Sep 19, 2016

GuilhemN deleted the FACTORIES branch September 19, 2016 19:30

hason mentioned this pull request Oct 21, 2016

[DependencyInjection] Fix FactoryReturnTypePass position in PassConfig #20263

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

javiereguiluz mentioned this pull request Nov 25, 2016

Add docs for FactoryReturnTypePass symfony/symfony-docs#7170

Closed

nicolas-grekas mentioned this pull request Jan 2, 2017

[DI] Optional class for named services #21133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Automatically detect the definitions class when possible #19191

[DependencyInjection] Automatically detect the definitions class when possible #19191

GuilhemN commented Jun 27, 2016

GuilhemN Jun 27, 2016

stof Jun 27, 2016

stof Jun 27, 2016

dunglas Jun 27, 2016

stof Jun 27, 2016

GuilhemN Jun 27, 2016

fabpot Jun 28, 2016

GuilhemN Jun 28, 2016

stof Jul 1, 2016

xabbuh Jul 5, 2016

GuilhemN Jul 5, 2016

GuilhemN commented Jul 6, 2016

fabpot Jul 8, 2016

GuilhemN commented Jul 11, 2016

dunglas commented Sep 6, 2016

fabpot Sep 14, 2016

GuilhemN Sep 15, 2016

xabbuh commented Sep 19, 2016

dunglas commented Sep 19, 2016 •

edited

Loading

fabpot commented Sep 19, 2016

nicolas-grekas commented Jan 2, 2017

GuilhemN commented Jan 2, 2017

[DependencyInjection] Automatically detect the definitions class when possible #19191

[DependencyInjection] Automatically detect the definitions class when possible #19191

Conversation

GuilhemN commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN commented Jul 6, 2016

Choose a reason for hiding this comment

GuilhemN commented Jul 11, 2016

dunglas commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Sep 19, 2016

dunglas commented Sep 19, 2016 • edited Loading

fabpot commented Sep 19, 2016

nicolas-grekas commented Jan 2, 2017

GuilhemN commented Jan 2, 2017

dunglas commented Sep 19, 2016 •

edited

Loading