[DependencyInjection] Add type-hints to interfaces and implementations. #32266

derrabus · 2019-06-28T16:30:45Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#32179
License	MIT
Doc PR	N/A

Part II for DependencyInjection, concentrating on the main interfaces, ContainerBuiler and related classes.

derrabus · 2019-06-28T17:19:34Z

src/Symfony/Component/DependencyInjection/ContainerInterface.php

@@ -51,7 +48,7 @@ public function set($id, $service);
     *
     * @see Reference
     */
-    public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE);
+    public function get($id, int $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE);


Note: We cannot add string $id because we would violate the PSR interface otherwise.

src/Symfony/Component/DependencyInjection/Definition.php

…getInEdges()/getOutEdges() (derrabus) This PR was merged into the 3.4 branch. Discussion ---------- [DependencyInjection] Annotated correct return type for getInEdges()/getOutEdges() | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A This changed annotation was quite helpful when analyzing `PhpDumper` for #32266. Commits ------- 28882f5 Annotated correct return type for getInEdges()/getOutEdges().

fabpot · 2019-07-05T05:10:15Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -1334,7 +1336,7 @@ public function getParameterBag()

 EOF;
        if (!$this->asFiles) {
-            $code = preg_replace('/^.*buildParameters.*\n.*\n.*\n/m', '', $code);
+            $code = preg_replace('/^.*buildParameters.*\n.*\n.*\n\n?/m', '', $code);


Should be reverted as it does not look related to the purpose of this PR, is it?

This class generates implementations of the ContainerInterface that has been changed with this PR. This is why I've adjusted the code template above to add the new type declarations and remove the now-obsolete type cast.

This preg_replace call removes an if block from the generated code under certain circumstances. Because I've removed the type cast, this operation would leave an empty line at the beginning of the method, like this:

public function hasParameter(string $name) { return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters); }

I've adjusted the regular expression, so that this extra new line is also removed in case its there.

src/Symfony/Component/DependencyInjection/Definition.php

src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/ShareTrait.php

src/Symfony/Component/DependencyInjection/Definition.php

Tobion · 2019-07-05T18:32:08Z

There are still several uneccessary string casts left, e.g.

symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Line 507 in 492df5e

if (isset($this->definitions[$id = (string) $id])) {
symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Line 859 in 492df5e

return isset($this->aliasDefinitions[$id = (string) $id]);
symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Line 960 in 492df5e

$id = (string) $id;

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Tobion · 2019-07-05T18:51:36Z

There seems to be something wrong in the failing tests testBindScalarValueToControllerArgument and testRegistersTraceableBusesToCollector

derrabus · 2019-07-05T18:59:23Z

I'll look into it.

…tances as class name to Definition (derrabus) This PR was merged into the 4.4 branch. Discussion ---------- [DependencyInjection] Deprecated passing Parameter instances as class name to Definition | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #32279 | License | MIT | Doc PR | N/A This PR deprecates the undocumented possibility to use a `Parameter` instance instead of a string as class name for a `Definition`. This was discovered while working on #32266. Commits ------- edfc9d6 Deprecated passing Parameter instances as class name to Definition.

derrabus · 2019-07-06T17:13:48Z

All right, so the failing test assumes that a service can be a scalar value.

symfony/src/Symfony/Component/HttpKernel/Tests/DependencyInjection/RegisterControllerArgumentLocatorsPassTest.php

Line 348 in 6811aaa

$this->assertSame('foo_val', $container->get((string) $reference));

All docblocks on the DI classes and interfaces however promise me that services are always objects.

symfony/src/Symfony/Component/DependencyInjection/ContainerInterface.php

Line 37 in 6811aaa

* @param object $service The service instance

symfony/src/Symfony/Component/DependencyInjection/ContainerInterface.php

Line 47 in 6811aaa

* @return object The associated service

symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Line 1641 in 6811aaa

* @param object $service

I trusted these docblocks and used object type-hints for services and those make the test mentioned by @Tobion fail. Interestingly enough, all tests of the DI component itself are green with the object type-hints. The failing test I linked above comes from the HttpKernel component.

I'm unsure if scalar services should be a thing, but they're possible at the moment, at least to a certain degree. In order to move forward here, I'll remove the object type-hints for now, so that we can decide afterwards if we either want to deprecate scalar services or update the docblocks.

Tobion · 2019-07-06T17:46:32Z

According to PSR-11 it could also be a scalar value.

use of a container to obtain objects and parameters

But that seems strange and not the original contract of the symfony container. Also parameters are not fetched individually from the container but through a ParameterBag in Symfony which makes more sense to me.

derrabus · 2019-07-06T18:26:22Z

The remaining failing tests are unrelated to my changes.

Status: Needs Review

derrabus · 2019-07-06T18:38:12Z

Follow-up issue: #32411

Tobion · 2019-07-06T18:55:16Z

Thank you @derrabus.

…mplementations. (derrabus) This PR was merged into the 5.0-dev branch. Discussion ---------- [DependencyInjection] Add type-hints to interfaces and implementations. | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #32179 | License | MIT | Doc PR | N/A Part II for DependencyInjection, concentrating on the main interfaces, `ContainerBuiler` and related classes. Commits ------- 68f9f27 [DependencyInjection] Add type-hints to interfaces and implementations.

carsonbot added Status: Needs Review DependencyInjection labels Jun 28, 2019

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from d3df351 to 4342375 Compare June 28, 2019 16:36

derrabus commented Jun 28, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/Definition.php Outdated Show resolved Hide resolved

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from 4342375 to 1b401f9 Compare June 28, 2019 22:10

derrabus marked this pull request as ready for review June 28, 2019 22:13

derrabus mentioned this pull request Jun 28, 2019

[5.0] Add parameter type-hints #32179

Closed

57 tasks

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from 1b401f9 to f421681 Compare June 29, 2019 15:39

derrabus mentioned this pull request Jun 29, 2019

[DependencyInjection] The class of a service definition might be passed as parameter #32279

Closed

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from f421681 to 13099b9 Compare June 29, 2019 16:41

derrabus mentioned this pull request Jun 29, 2019

[DependencyInjection] Annotated correct return type for getInEdges()/getOutEdges() #32281

Merged

nicolas-grekas added this to the 5.0 milestone Jun 30, 2019

fabpot requested changes Jul 5, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 5, 2019

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch 2 times, most recently from 9a63519 to 492df5e Compare July 5, 2019 09:15

derrabus mentioned this pull request Jul 5, 2019

[DependencyInjection] Deprecated passing Parameter instances as class name to Definition #32390

Merged

Tobion reviewed Jul 5, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/ShareTrait.php Show resolved Hide resolved

Tobion reviewed Jul 5, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/Definition.php Show resolved Hide resolved

Tobion reviewed Jul 5, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/ContainerBuilder.php Show resolved Hide resolved

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from 492df5e to d44d0a1 Compare July 5, 2019 18:51

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from d44d0a1 to d45497b Compare July 6, 2019 16:02

[DependencyInjection] Add type-hints to interfaces and implementations.

68f9f27

derrabus force-pushed the improvement/type-hints-on-di-interfaces branch from d45497b to 68f9f27 Compare July 6, 2019 18:04

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jul 6, 2019

derrabus mentioned this pull request Jul 6, 2019

[DependencyInjection] Do we support scalar services? #32411

Closed

Tobion approved these changes Jul 6, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 6, 2019

Tobion merged commit 68f9f27 into symfony:master Jul 6, 2019

derrabus deleted the improvement/type-hints-on-di-interfaces branch July 6, 2019 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Add type-hints to interfaces and implementations. #32266

[DependencyInjection] Add type-hints to interfaces and implementations. #32266

derrabus commented Jun 28, 2019

derrabus Jun 28, 2019

fabpot Jul 5, 2019

derrabus Jul 5, 2019

Tobion commented Jul 5, 2019 •

edited

Loading

Tobion commented Jul 5, 2019

derrabus commented Jul 5, 2019

derrabus commented Jul 6, 2019

Tobion commented Jul 6, 2019

derrabus commented Jul 6, 2019

derrabus commented Jul 6, 2019

Tobion commented Jul 6, 2019

[DependencyInjection] Add type-hints to interfaces and implementations. #32266

[DependencyInjection] Add type-hints to interfaces and implementations. #32266

Conversation

derrabus commented Jun 28, 2019

derrabus Jun 28, 2019

Choose a reason for hiding this comment

fabpot Jul 5, 2019

Choose a reason for hiding this comment

derrabus Jul 5, 2019

Choose a reason for hiding this comment

Tobion commented Jul 5, 2019 • edited Loading

Tobion commented Jul 5, 2019

derrabus commented Jul 5, 2019

derrabus commented Jul 6, 2019

Tobion commented Jul 6, 2019

derrabus commented Jul 6, 2019

derrabus commented Jul 6, 2019

Tobion commented Jul 6, 2019

Tobion commented Jul 5, 2019 •

edited

Loading