Skip to content

Remove some container injections in favor of service locators #21625

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

Merged
merged 2 commits into from
Feb 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions UPGRADE-3.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,22 @@ FrameworkBundle
deprecated and will be removed in 4.0. Use the `Symfony\Component\Form\DependencyInjection\FormPass`
class instead.

* The `Symfony\Bundle\FrameworkBundle\EventListener\SessionListener` class has been
deprecated and will be removed in 4.0. Use the `Symfony\Component\HttpKernel\EventListener\SessionListener`
class instead.

* The `Symfony\Bundle\FrameworkBundle\EventListener\TestSessionListener` class has been
deprecated and will be removed in 4.0. Use the `Symfony\Component\HttpKernel\EventListener\TestSessionListener`
class instead.

HttpKernel
-----------

* The `Psr6CacheClearer::addPool()` method has been deprecated. Pass an array of pools indexed
by name to the constructor instead.

* The `LazyLoadingFragmentHandler::addRendererService()` method has been deprecated and
will be removed in 4.0.

Process
-------
Expand Down Expand Up @@ -127,6 +138,12 @@ TwigBridge
* The `TwigRendererEngine::setEnvironment()` method has been deprecated and will be removed
in 4.0. Pass the Twig Environment as second argument of the constructor instead.

TwigBundle
----------

* The `ContainerAwareRuntimeLoader` class has been deprecated and will be removed in 4.0.
Use the Twig `Twig_ContainerRuntimeLoader` class instead.

Workflow
--------

Expand Down
15 changes: 15 additions & 0 deletions UPGRADE-4.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ FrameworkBundle
* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\FormPass` class has been
removed. Use the `Symfony\Component\Form\DependencyInjection\FormPass` class instead.

* The `Symfony\Bundle\FrameworkBundle\EventListener\SessionListener` class has been removed.
Use the `Symfony\Component\HttpKernel\EventListener\SessionListener` class instead.

* The `Symfony\Bundle\FrameworkBundle\EventListener\TestSessionListener` class has been
removed. Use the `Symfony\Component\HttpKernel\EventListener\TestSessionListener`
class instead.

HttpFoundation
---------------

Expand Down Expand Up @@ -229,6 +236,8 @@ HttpKernel

* The `Psr6CacheClearer::addPool()` method has been removed. Pass an array of pools indexed
by name to the constructor instead.

* The `LazyLoadingFragmentHandler::addRendererService()` method has been removed.

Ldap
----
Expand Down Expand Up @@ -285,6 +294,12 @@ Translation

* Removed the backup feature from the file dumper classes.

TwigBundle
----------

* The `ContainerAwareRuntimeLoader` class has been removed. Use the
Twig `Twig_ContainerRuntimeLoader` class instead.

TwigBridge
----------

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ CHANGELOG
* Added configurable paths for validation files
* Deprecated `SerializerPass`, use `Symfony\Component\Serializer\DependencyInjection\SerializerPass` instead
* Deprecated `FormPass`, use `Symfony\Component\Form\DependencyInjection\FormPass` instead
* Deprecated `SessionListener`
* Deprecated `TestSessionListener`

3.2.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,31 +12,16 @@
namespace Symfony\Bundle\FrameworkBundle\EventListener;

use Symfony\Component\HttpKernel\EventListener\SessionListener as BaseSessionListener;
use Symfony\Component\DependencyInjection\ContainerInterface;

@trigger_error(sprintf('The %s class is deprecated since version 3.3 and will be removed in 4.0. Use %s instead.', SessionListener::class, BaseSessionListener::class), E_USER_DEPRECATED);

/**
* Sets the session in the request.
*
* @author Fabien Potencier <fabien@symfony.com>
*
* @deprecated since version 3.3, to be removed in 4.0. Use {@link BaseSessionListener} instead
*/
class SessionListener extends BaseSessionListener
{
/**
* @var ContainerInterface
*/
private $container;

public function __construct(ContainerInterface $container)
{
$this->container = $container;
}

protected function getSession()
{
if (!$this->container->has('session')) {
return;
}

return $this->container->get('session');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,19 @@

namespace Symfony\Bundle\FrameworkBundle\EventListener;

use Symfony\Component\HttpKernel\EventListener\TestSessionListener as BaseTestSessionListener;
@trigger_error(sprintf('The %s class is deprecated since version 3.3 and will be removed in 4.0. Use Symfony\Component\HttpKernel\EventListener\TestSessionListener instead.', TestSessionListener::class), E_USER_DEPRECATED);

use Symfony\Component\HttpKernel\EventListener\AbstractTestSessionListener;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
* TestSessionListener.
*
* @author Fabien Potencier <fabien@symfony.com>
*
* @deprecated since version 3.3, to be removed in 4.0.
*/
class TestSessionListener extends BaseTestSessionListener
class TestSessionListener extends AbstractTestSessionListener
{
protected $container;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do getter injection on this one very easily!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's what I mean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FrameworkBundle class was just doing getter injection implemented manually.

Copy link
Member Author

@chalasr chalasr Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, added as last commit a4a4b3b

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReflectionClass::newInstanceWithoutConstructor breaks on it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is newInstanceWithoutConstructor called inside ContainerBuilder or inside the dumped container?

Copy link
Member

@stof stof Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<argument type="service" id="request_stack" />
<argument>%kernel.debug%</argument>
</service>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,11 @@

<service id="session.handler.write_check" class="Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler" public="false" />

<service id="session_listener" class="Symfony\Bundle\FrameworkBundle\EventListener\SessionListener">
<service id="session_listener" class="Symfony\Component\HttpKernel\EventListener\SessionListener">
<tag name="kernel.event_subscriber" />
<argument type="service" id="service_container" />
<argument type="service-locator">
<argument key="session" type="service" id="session" on-invalid="null" />
</argument>
</service>

<service id="session.save_listener" class="Symfony\Component\HttpKernel\EventListener\SaveSessionListener">
Expand Down
6 changes: 4 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/test.xml
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@

<service id="test.client.cookiejar" class="Symfony\Component\BrowserKit\CookieJar" shared="false" />

<service id="test.session.listener" class="Symfony\Bundle\FrameworkBundle\EventListener\TestSessionListener">
<argument type="service" id="service_container" />
<service id="test.session.listener" class="Symfony\Component\HttpKernel\EventListener\TestSessionListener">
<tag name="kernel.event_subscriber" />
<argument type="service-locator">
<argument key="session" type="service" id="session" on-invalid="null" />
</argument>
</service>
</services>
</container>
5 changes: 5 additions & 0 deletions src/Symfony/Bundle/TwigBundle/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

3.3.0
-----

* Deprecated `ContainerAwareRuntimeLoader`

2.7.0
-----

Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Bundle/TwigBundle/ContainerAwareRuntimeLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,17 @@

namespace Symfony\Bundle\TwigBundle;

@trigger_error(sprintf('The %s class is deprecated since version 3.3 and will be removed in 4.0. Use the Twig_ContainerRuntimeLoader class instead.'), ContainerAwareRuntimeLoader::class);

use Psr\Log\LoggerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
* Loads Twig extension runtimes via the service container.
*
* @author Fabien Potencier <fabien@symfony.com>
*
* @deprecated since version 3.3, will be removed in 4.0. Use \Twig_ContainerRuntimeLoader instead.
*/
class ContainerAwareRuntimeLoader implements \Twig_RuntimeLoaderInterface
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@

namespace Symfony\Bundle\TwigBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Reference;

/**
* Registers Twig runtime services.
Expand All @@ -31,17 +32,13 @@ public function process(ContainerBuilder $container)
foreach ($container->findTaggedServiceIds('twig.runtime') as $id => $attributes) {
$def = $container->getDefinition($id);

if (!$def->isPublic()) {
throw new InvalidArgumentException(sprintf('The service "%s" must be public as it can be lazy-loaded.', $id));
}

if ($def->isAbstract()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed too, this is checked in CheckReferenceValidityPass.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitated, thanks for confirming.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isAbstract() checks removed

throw new InvalidArgumentException(sprintf('The service "%s" must not be abstract as it can be lazy-loaded.', $id));
continue;
}

$mapping[$def->getClass()] = $id;
$mapping[$def->getClass()] = new Reference($id);
}

$definition->replaceArgument(1, $mapping);
$definition->replaceArgument(0, new ServiceLocatorArgument($mapping));
}
}
6 changes: 2 additions & 4 deletions src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,8 @@
<argument /> <!-- thousands separator, set in TwigExtension -->
</service>

<service id="twig.runtime_loader" class="Symfony\Bundle\TwigBundle\ContainerAwareRuntimeLoader" public="false">
<argument type="service" id="service_container" />
<argument type="collection" /> <!-- the mapping between class names and service names -->
<argument type="service" id="logger" on-invalid="null" />
<service id="twig.runtime_loader" class="Twig_ContainerRuntimeLoader" public="false">
<argument /> <!-- runtime locator -->
</service>
</services>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Bundle\TwigBundle\ContainerAwareRuntimeLoader;

/**
* @group legacy
*/
class ContainerAwareRuntimeLoaderTest extends TestCase
{
public function testLoad()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public function testRuntimeLoader()
$container->compile();

$loader = $container->getDefinition('twig.runtime_loader');
$args = $loader->getArgument(1);
$args = $loader->getArgument(0)->getValues();
$this->assertArrayHasKey('Symfony\Bridge\Twig\Form\TwigRenderer', $args);
$this->assertArrayHasKey('FooClass', $args);
$this->assertContains('twig.form.renderer', $args);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/TwigBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.32|^2.2"
},
"require-dev": {
"symfony/asset": "~2.8|~3.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
* Compiler pass to register tagged services for an event dispatcher.
Expand Down Expand Up @@ -105,8 +106,8 @@ public function process(ContainerBuilder $container)
}
$container->addObjectResource($class);

$r = new \ReflectionClass($class);
$extractingDispatcher->addSubscriber($r->newInstanceWithoutConstructor());
ExtractingEventDispatcher::$subscriber = $class;
$extractingDispatcher->addSubscriber($extractingDispatcher);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice trick 😄

foreach ($extractingDispatcher->listeners as $args) {
$args[1] = new ClosureProxyArgument($id, $args[1]);
$definition->addMethodCall('addListener', $args);
Expand All @@ -119,12 +120,21 @@ public function process(ContainerBuilder $container)
/**
* @internal
*/
class ExtractingEventDispatcher extends EventDispatcher
class ExtractingEventDispatcher extends EventDispatcher implements EventSubscriberInterface
{
public $listeners = array();

public static $subscriber;

public function addListener($eventName, $listener, $priority = 0)
{
$this->listeners[] = array($eventName, $listener[1], $priority);
}

public static function getSubscribedEvents()
{
$callback = array(self::$subscriber, 'getSubscribedEvents');

return $callback();
}
}
7 changes: 7 additions & 0 deletions src/Symfony/Component/HttpKernel/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
CHANGELOG
=========

3.3.0
-----

* Deprecated `LazyLoadingFragmentHandler::addRendererService()`
* Added `SessionListener`
* Added `TestSessionListener`

3.2.0
-----

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@

namespace Symfony\Component\HttpKernel\DependencyInjection;

use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
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\HttpKernel\Fragment\FragmentRendererInterface;

/**
Expand Down Expand Up @@ -43,14 +45,11 @@ public function process(ContainerBuilder $container)
}

$definition = $container->getDefinition($this->handlerService);
$renderers = array();
foreach ($container->findTaggedServiceIds($this->rendererTag) as $id => $tags) {
$def = $container->getDefinition($id);
if (!$def->isPublic()) {
throw new InvalidArgumentException(sprintf('The service "%s" must be public as fragment renderer are lazy-loaded.', $id));
}

if ($def->isAbstract()) {
throw new InvalidArgumentException(sprintf('The service "%s" must not be abstract as fragment renderer are lazy-loaded.', $id));
continue;
}

$class = $container->getParameterBag()->resolveValue($def->getClass());
Expand All @@ -63,8 +62,10 @@ public function process(ContainerBuilder $container)
}

foreach ($tags as $tag) {
$definition->addMethodCall('addRendererService', array($tag['alias'], $id));
$renderers[$tag['alias']] = new Reference($id);
}
}

$definition->replaceArgument(0, new ServiceLocatorArgument($renderers));
}
}
Loading