Remove some container injections in favor of service locators #21625

chalasr · 2017-02-15T23:16:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21553 (comment)
License	MIT
Doc PR	n/a

chalasr · 2017-02-16T12:56:20Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

+            } else {
+                $logger = null;
+            }
+        }


Suggestions to make this deprecation better are welcomed.

missing type check when the 2nd arg is not an array (LoggerInterface|null)

hhamon · 2017-02-16T13:31:03Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

-        $this->mapping = $mapping;
+        //  BC 3.x, to be removed in 4.0, re-adding the LoggerInterface typehint to the $logger argument
+        if (is_array($logger)) {
+            @trigger_error(sprintf('The second argument of %s() is deprecated since symfony 3.3 and will be removed, passing something else than a %s instance will trigger an error in 4.0.', __METHOD__, LoggerInterface::class), E_USER_DEPRECATED);


/symfony 3.3/Symfony 3.3/

hhamon · 2017-02-16T13:31:41Z

src/Symfony/Bundle/TwigBundle/Tests/ContainerAwareRuntimeLoaderTest.php

+
+    /**
+     * @group legacy
+     * @expectedDeprecation The second argument of Symfony\Bundle\TwigBundle\ContainerAwareRuntimeLoader::__construct() is deprecated since symfony 3.3 and will be removed, passing something else than a Psr\Log\LoggerInterface instance will trigger an error in 4.0.


/symfony 3.3/Symfony 3.3/

nicolas-grekas · 2017-02-17T06:34:20Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/fragment_renderer.xml

@@ -11,7 +11,7 @@

    <services>
        <service id="fragment.handler" class="Symfony\Component\HttpKernel\DependencyInjection\LazyLoadingFragmentHandler">
-            <argument type="service" id="service_container" />
+            <argument /> <!-- fragment renderer locator -->


you can do getter injection on this one very easily!

are you talking about getSession() in the session listener instead? If no, could you elaborate?

yes, that's what I mean

@chalasr yeah, this seems to be on the wrong service.

for the SessionListener, I'm in favor of leaving the FrameworkBundle class unchanged, deprecating it, and using getter injection on the component class itself.

The FrameworkBundle class was just doing getter injection implemented manually.

sorry, added as last commit a4a4b3b

ReflectionClass::newInstanceWithoutConstructor breaks on it

is newInstanceWithoutConstructor called inside ContainerBuilder or inside the dumped container?

@nicolas-grekas in the compiler pass, when registering the subscriber in a dummy event dispatcher to collect subscribed events and register it lazily.

however, I don't even understand why we do this: In Symfony stable releases, the ContainerAwareEventDispatcher calls the static getSubscribedEvents statically. We don't need an instance for that.

that's because of https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/EventDispatcher.php#L127 which requires an instance
but this PR now has a fix you will like :)

nicolas-grekas · 2017-02-17T06:38:13Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

-        $this->mapping = $mapping;
+        //  BC 3.x, to be removed in 4.0, re-adding the LoggerInterface typehint to the $logger argument
+        if (is_array($logger)) {
+            @trigger_error(sprintf('The second argument of %s() is deprecated since version 3.3 and will be removed, passing something else than a %s instance will trigger an error in 4.0.', __METHOD__, LoggerInterface::class), E_USER_DEPRECATED);


"The $mapping argument..." (the 2nd won't be removed since it will expect the logger)

nicolas-grekas · 2017-02-17T06:40:15Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

+            } else {
+                $logger = null;
+            }
+        }


missing type check when the 2nd arg is not an array (LoggerInterface|null)

nicolas-grekas · 2017-02-17T06:41:09Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

+        if (is_array($logger)) {
+            @trigger_error(sprintf('The second argument of %s() is deprecated since version 3.3 and will be removed, passing something else than a %s instance will trigger an error in 4.0.', __METHOD__, LoggerInterface::class), E_USER_DEPRECATED);
+
+            if (2 < func_num_args() && func_get_arg(3) instanceof LoggerInterface) {


not needed if the constructor signature is changed to
__construct(ContainerInterface $container, $mapping = null, LoggerInterface $logger = null)

nicolas-grekas · 2017-02-17T06:42:00Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

    }

    /**
     * {@inheritdoc}
     */
    public function load($class)
    {
-        if (isset($this->mapping[$class])) {


when $mapping is provided as an array, we should still use it

nicolas-grekas · 2017-02-17T06:43:36Z

src/Symfony/Component/HttpKernel/DependencyInjection/LazyLoadingFragmentHandler.php

@@ -42,23 +42,24 @@ public function __construct(ContainerInterface $container, RequestStack $request
    /**
     * Adds a service as a fragment renderer.
     *
+     * @deprecated since version 3.3, to be removed in 4.0


should always be put last, after @param

stof · 2017-02-17T09:09:54Z

src/Symfony/Bundle/TwigBundle/Tests/ContainerAwareRuntimeLoaderTest.php

+     */
+    public function testSecondConstructorArgumentIsDeprecated()
+    {
+        $loader = new ContainerAwareRuntimeLoader($this->getMockBuilder(ContainerInterface::class)->getMock(), array());


you also need to test that the existing behavior (using a mapping) keeps working. Copy the existing testLoad method for that.

stof · 2017-02-17T09:10:55Z

src/Symfony/Component/HttpKernel/DependencyInjection/LazyLoadingFragmentHandler.php

     * @param string $name     The service name
     * @param string $renderer The render service id
     */
    public function addRendererService($name, $renderer)
    {
-        $this->rendererIds[$name] = $renderer;


removing the implementation is wrong. It means that the feature does not work anymore, which is a BC break

stof · 2017-02-17T09:11:34Z

src/Symfony/Component/HttpKernel/DependencyInjection/LazyLoadingFragmentHandler.php

-        if (isset($this->rendererIds[$renderer])) {
-            $this->addRenderer($this->container->get($this->rendererIds[$renderer]));
-
-            unset($this->rendererIds[$renderer]);


you must keep using $this->rendererIds for the BC layer

stof · 2017-02-17T09:12:16Z

src/Symfony/Component/HttpKernel/Tests/DependencyInjection/LazyLoadingFragmentHandlerTest.php

+    public function testAddRendererServiceIsDeprecated()
+    {
+        $handler = new LazyLoadingFragmentHandler($this->getMockBuilder('Psr\Container\ContainerInterface')->getMock(), $this->getMockBuilder('Symfony\Component\HttpFoundation\RequestStack')->getMock());
+        $handler->addRendererService('foo', 'bar');


you must keep the existing tests of the class here, to ensure that the 3.2 way of using the class keeps working

stof · 2017-02-17T09:13:58Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/RuntimeLoaderPass.php

@@ -14,6 +14,8 @@
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;


We tend to sort new use statements (without touching existing ones, to avoid a nightmare when merging branches). So please move this one at the beginning

stof · 2017-02-17T09:14:48Z

src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php

@@ -11,8 +11,8 @@

 namespace Symfony\Bundle\TwigBundle;

+use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;


@fabpot as this class now depends only on PSR classes, should we add it in Twig itself ?

Indeed, that's probably a good idea.

ok I'll do a twig PR in parallel.

@stof This means deprecating this class and bump the twig requirement to 2.x in TwigBundle to use the twig class instead, right?

updated to use the twig class, it will break tests until twigphp/Twig#2401 is merged and released

chalasr · 2017-02-17T09:49:02Z

Thanks for the reviews.

Status: needs work

fabpot · 2017-02-17T18:27:59Z

src/Symfony/Bundle/TwigBundle/composer.json

@@ -21,7 +21,7 @@
        "symfony/twig-bridge": "^3.2.1",
        "symfony/http-foundation": "~2.8|~3.0",
        "symfony/http-kernel": "~2.8.16|~3.1.9|^3.2.2",
-        "twig/twig": "~1.28|~2.0"
+        "twig/twig": "~2.2"


That's not possible. We cannot drop support for Twig 1.x. But the good news is that we are still maintaining Twig 1.x and 2.x. So the constraint can be ^1.32|^2.2.

constraint fixed

fabpot · 2017-02-17T18:37:42Z

src/Symfony/Bundle/TwigBundle/composer.json

@@ -21,7 +21,7 @@
        "symfony/twig-bridge": "^3.2.1",
        "symfony/http-foundation": "~2.8|~3.0",
        "symfony/http-kernel": "~2.8.16|~3.1.9|^3.2.2",
-        "twig/twig": "~1.28|~2.0"
+        "twig/twig": "^1.29|^2.2"


Sorry, should be 1.32.

This PR was merged into the 1.x branch. Discussion ---------- Add ContainerRuntimeLoader Next to symfony/symfony#21625 (comment) Commits ------- 964a51b Add ContainerRuntimeLoader

chalasr · 2017-02-26T10:40:16Z

*SessionListener deprecated and moved to HttpKernel, note that the naming change was required.
Status: needs review

…ument type (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Remove experimental status from service-locator argument type | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21625 (comment), #21625 (comment), #21710 | License | MIT The `service-locator` argument type is not controversial to me. We know its scope, nothing really surprising, just a map of services to be lazily loaded like `iterator` is (which is not experimental) but keyed. About its api, it's just PSR-11 restricted to objects, nothing that can't be changed safely in the future. As stated in #21625 (comment), it proven its usefulness already. I think what we were looking for by flagging it experimental is just to see it in action, we've 3 opened PRs for that (#21625, #21690, #21730). This allows introducing deprecations for making use of the feature in the core, thus unlocks #21625 and #21690. Commits ------- 46dc47a [DI] Remove experimental status from service-locator argument type

…ument type (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Remove experimental status from service-locator argument type | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#21625 (comment), symfony/symfony#21625 (comment), #21710 | License | MIT The `service-locator` argument type is not controversial to me. We know its scope, nothing really surprising, just a map of services to be lazily loaded like `iterator` is (which is not experimental) but keyed. About its api, it's just PSR-11 restricted to objects, nothing that can't be changed safely in the future. As stated in symfony/symfony#21625 (comment), it proven its usefulness already. I think what we were looking for by flagging it experimental is just to see it in action, we've 3 opened PRs for that (#21625, #21690, #21730). This allows introducing deprecations for making use of the feature in the core, thus unlocks #21625 and #21690. Commits ------- 46dc47af11 [DI] Remove experimental status from service-locator argument type

nicolas-grekas

👍

fabpot · 2017-02-28T02:18:43Z

Thank you @chalasr.

…ocators (nicolas-grekas, chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- Remove some container injections in favor of service locators | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21553 (comment) | License | MIT | Doc PR | n/a Commits ------- 8293b75 Replace some container injections by service locators 0be9ea8 [EventDispatcher] Fix abstract event subscribers registration

fabpot · 2017-02-28T04:22:48Z

This PR breaks the test suite of SensioFrameworkExtraBundle. @chalasr Can you have a look at it?

chalasr · 2017-02-28T09:06:57Z

@fabpot see #21794

…ence values (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Fix ServiceLocatorArgument::setValues() for non-reference values | Q | A | ------------- | --- | Branch? | master | Fixed tickets | #21625 (comment) | Tests pass? | yes | License | MIT `ResolveInvalidReferencesPass` [calls `setValues()`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php#L91) with resolved invalid reference (null), the `Reference` type check should occurs at construction only. Commits ------- 256b836 [DI] Fix ServiceLocatorArgument::setValues() for non-reference values

…sses (chalasr) This PR was merged into the 4.0-dev branch. Discussion ---------- Remove deprecated container injections and compiler passes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~~#21451 #21625 #21284 #22010~~ #22805 | License | MIT | Doc PR | n/a Commits ------- 16a2fcf Remove deprecated container injections and compiler passes

carsonbot added Status: Needs Review Feature Deprecation labels Feb 15, 2017

chalasr force-pushed the remove-container-injections branch 4 times, most recently from 6d9025c to bee8bac Compare February 16, 2017 12:53

chalasr commented Feb 16, 2017

View reviewed changes

chalasr force-pushed the remove-container-injections branch from bee8bac to 404200a Compare February 16, 2017 13:03

hhamon reviewed Feb 16, 2017

View reviewed changes

chalasr force-pushed the remove-container-injections branch 2 times, most recently from 1d3ef15 to 2675b30 Compare February 16, 2017 14:35

nicolas-grekas reviewed Feb 17, 2017

View reviewed changes

stof requested changes Feb 17, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 17, 2017

chalasr force-pushed the remove-container-injections branch from 2675b30 to 3e0b7f6 Compare February 17, 2017 15:58

chalasr mentioned this pull request Feb 17, 2017

Add PSR-11 compatible ContainerRuntimeLoader twigphp/Twig#2401

Merged

chalasr force-pushed the remove-container-injections branch from 3e0b7f6 to c89b788 Compare February 17, 2017 18:20

fabpot reviewed Feb 17, 2017

View reviewed changes

chalasr force-pushed the remove-container-injections branch from c89b788 to 097856e Compare February 17, 2017 18:34

fabpot reviewed Feb 17, 2017

View reviewed changes

chalasr force-pushed the remove-container-injections branch 3 times, most recently from a4a4b3b to 550f877 Compare February 17, 2017 19:03

chalasr force-pushed the remove-container-injections branch 3 times, most recently from 85c30a0 to 961234e Compare February 17, 2017 20:11

chalasr force-pushed the remove-container-injections branch 2 times, most recently from ce55cd6 to 2e5f25e Compare February 26, 2017 10:15

chalasr changed the title ~~Remove some container injections in favor of service locators/getter injection~~ Remove some container injections in favor of service locators Feb 26, 2017

chalasr force-pushed the remove-container-injections branch 2 times, most recently from c37ac4d to f891b7e Compare February 26, 2017 10:25

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 26, 2017

chalasr force-pushed the remove-container-injections branch 4 times, most recently from 7d6f58f to 23662b2 Compare February 26, 2017 12:50

Replace some container injections by service locators

8293b75

chalasr force-pushed the remove-container-injections branch from 23662b2 to 8293b75 Compare February 26, 2017 13:01

nicolas-grekas approved these changes Feb 27, 2017

View reviewed changes

fabpot merged commit 8293b75 into symfony:master Feb 28, 2017

chalasr deleted the remove-container-injections branch February 28, 2017 08:47

chalasr mentioned this pull request Feb 28, 2017

[DI] Fix ServiceLocatorArgument::setValues() for non-reference values #21794

Merged

GromNaN mentioned this pull request Mar 26, 2017

Restore SessionListener class for backward compatibility with Silex #22171

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

chalasr mentioned this pull request May 18, 2017

Remove deprecated container injections and compiler passes #22749

Merged

mbabker mentioned this pull request May 20, 2017

[TwigBundle] Remove ContainerAwareRuntimeLoader #22805

Closed

Remove some container injections in favor of service locators #21625

Remove some container injections in favor of service locators #21625

Conversation

chalasr commented Feb 15, 2017 • edited Loading

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

chalasr Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof Feb 17, 2017 • edited Loading

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

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

chalasr Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

chalasr commented Feb 17, 2017

fabpot Feb 17, 2017 • edited Loading

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

chalasr commented Feb 26, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Feb 28, 2017

fabpot commented Feb 28, 2017

chalasr commented Feb 28, 2017

chalasr commented Feb 15, 2017 •

edited

Loading

chalasr Feb 17, 2017 •

edited

Loading

stof Feb 17, 2017 •

edited

Loading

chalasr Feb 17, 2017 •

edited

Loading

fabpot Feb 17, 2017 •

edited

Loading