-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,11 @@ | ||
CHANGELOG | ||
========= | ||
|
||
3.3.0 | ||
----- | ||
|
||
* Deprecated `ContainerAwareRuntimeLoader` | ||
|
||
2.7.0 | ||
----- | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be removed too, this is checked in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hesitated, thanks for confirming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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(); | ||
} | ||
} |
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.
you can do getter injection on this one very easily!
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.
are you talking about
getSession()
in the session listener instead? If no, could you elaborate?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.
yes, that's what I mean
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.
@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.
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.
The FrameworkBundle class was just doing getter injection implemented manually.
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.
sorry, added as last commit a4a4b3b
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.
ReflectionClass::newInstanceWithoutConstructor
breaks on itThere 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.
is newInstanceWithoutConstructor called inside ContainerBuilder or inside the dumped container?
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.
@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.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 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 :)