8000 [EventSubscriber] Infer subscribed events from listener parameters by derrabus · Pull Request #30801 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[EventSubscriber] Infer subscribed events from listener parameters #30801

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

Closed
Closed
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
Infer events a subscriber subscribes to from listener parameters.
  • Loading branch information
derrabus committed Mar 31, 2019
commit 359870e6a61040a4a7e739bb0ded1e7bb956af8d
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,9 @@ public static function getSubscribedEvents()

return $events;
}

protected function inferEventName($subscriber, $params): string
{
return parent::inferEventName($subscriber instanceof self ? self::$subscriber : $subscriber, $params);
}
}
41 changes: 41 additions & 0 deletions src/Symfony/Component/EventDispatcher/EventDispatcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ public function removeListener($eventName, $listener)
public function addSubscriber(EventSubscriberInterface $subscriber)
{
foreach ($subscriber->getSubscribedEvents() as $eventName => $params) {
if (\is_int($eventName)) {
$eventName = $this->inferEventName($subscriber, $params);
}
if (\is_string($params)) {
$this->addListener($eventName, [$subscriber, $params]);
} elseif (\is_string($params[0])) {
Expand All @@ -208,6 +211,9 @@ public function addSubscriber(EventSubscriberInterface $subscriber)
public function removeSubscriber(EventSubscriberInterface $subscriber)
{
foreach ($subscriber->getSubscribedEvents() as $eventName => $params) {
if (\is_int($eventName)) {
$eventName = $this->inferEventName($subscriber, $params);
}
if (\is_array($params) && \is_array($params[0])) {
foreach ($params as $listener) {
$this->removeListener($eventName, [$subscriber, $listener[0]]);
Expand Down Expand Up @@ -259,6 +265,41 @@ protected function doDispatch($listeners, $eventName, Event $event)
}
}

/**
* @param string|object $subscriber
* @param string|array $params
*
* @return string
*/
protected function inferEventName($subscriber, $params): string
Copy link
Member

Choose a reason for hiding this comment

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

this should be in the pass, i.e. compile-time only

Copy link
Member
@nicolas-grekas nicolas-grekas Apr 3, 2019

Choose a reason for hiding this comment

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

Hum, but this would mean getSubscribedEvents wouldn't by that decoupled anymore as the pass would be a requirement to allow bypassing the event name.
And on the other side, I don't like having the reflection-related logic in the main object.
I think I'm 👎 on this idea sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

this should be in the pass, i.e. compile-time only

The logic is executed on compile-time only – in the full-stack framework scenario. If I moved the reflection logic into the pass, this feature would become a DI-only feature as it would not work anymore when using the dispatcher standalone.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly what I meant also.
We just closed #30760 to save the code from doing more Reflection, even at compile time.
It would be consistent to close this one also, sorry again.

Copy link
Member Author

Choose a reason for hiding this comment

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

All right.

{
$methodName = \is_array($params) ? $params[0] : $pa 10000 rams;

try {
$parameters = (new \ReflectionMethod($subscriber, $methodName))->getParameters();
} catch (\ReflectionException $e) {
throw new \UnexpectedValueException(sprintf(
'Cannot infer event name for missing method "%s::%s".',
\is_object($subscriber) ? \get_class($subscriber) : $subscriber,
$methodName
), 0, $e);
}

if (
empty($parameters)
|| null === ($type = $parameters[0]->getType())
|| $type->isBuiltin()
) {
throw new \UnexpectedValueException(sprintf(
'Cannot infer event name for method "%s::%s". Please add a type-hint for the event calls to the first parameter of the method or configure the event name explicitly.',
\is_object($subscriber) ? \get_class($subscriber) : $subscriber,
$methodName
));
}

return $type->getName();
}

/**
* Sorts the internal list of listeners for the given event by priority.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ interface EventSubscriberInterface
* * ['eventName' => ['methodName', $priority]]
* * ['eventName' => [['methodName1', $priority], ['methodName2']]]
*
* Alternatively, the keys can be omitted. In that case, the event name is
* inferred from the type hint of the first parameter of the specified method.
*
* * ['methodName']
* * [['methodName1', $priority], 'methodName2']
*
* @return array The event names to listen to
*/
public static function getSubscribedEvents();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Contracts\EventDispatcher\Event;

class RegisterListenersPassTest extends TestCase
{
Expand Down Expand Up @@ -59,6 +61,30 @@ public function testValidEventSubscriber()
0,
],
],
[
'addListener',
[
MockEvent::class,
[new ServiceClosureArgument(new Reference('my_event_subscriber')), 'onExplicitlyConfiguredEvent'],
0,
],
],
[
'addListener',
[
MockEvent::class,
[new ServiceClosureArgument(new Reference('my_event_subscriber')), 'onMockEvent'],
0,
],
],
[
'addListener',
[
MockEvent::class,
[new ServiceClosureArgument(new Reference('my_event_subscriber')), 'onEarlyMockEvent'],
255,
],
],
];
$this->assertEquals($expectedCalls, $eventDispatcherDefinition->getMethodCalls());
}
Expand Down Expand Up @@ -112,6 +138,30 @@ public function testEventSubscriberResolvableClassName()
0,
],
],
[
'addListener',
[
MockEvent::class,
[new ServiceClosureArgument(new Reference('foo')), 'onExplicitlyConfiguredEvent'],
0,
],
],
[
'addListener',
[
MockEvent::class,
[new ServiceClosureArgument(new Reference('foo')), 'onMockEvent'],
0,
],
],
[
'addListener',
[
MockEvent::class,
[new ServiceClosureArgument(new Reference('foo')), 'onEarlyMockEvent'],
255,
],
],
];
$this->assertEquals($expectedCalls, $definition->getMethodCalls());
}
Expand Down Expand Up @@ -184,14 +234,29 @@ public function testInvokableEventListener()
}
}

class SubscriberService implements \Symfony\Component\EventDispatcher\EventSubscriberInterface
class SubscriberService implements EventSubscriberInterface
{
public static function getSubscribedEvents()
{
return [
'event' => 'onEvent',
MockEvent::class => 'onExplicitlyConfiguredEvent',
'onMockEvent',
['onEarlyMockEvent', 255],
];
}

public function onEarlyMockEvent(MockEvent $event): void
{
}

public function onMockEvent(MockEvent $event): void
{
}
}

class MockEvent extends Event
{
}

class InvokableListenerService
Expand Down
106 changes: 106 additions & 0 deletions src/Symfony/Component/EventDispatcher/Tests/EventDispatcherTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,49 @@ public function testAddSubscriberWithMultipleListeners()
$this->assertEquals('preFoo2', $listeners[0][1]);
}

public function testAddSubscriberWithoutEventName()
{
$eventSubscriber = new TestEventSubscriberWithoutEventName();
$this->dispatcher->addSubscriber($eventSubscriber);
$this->assertCount(2, $this->dispatcher->getListeners(MockEvent::class));
}

public function testImplicitSubscriberWithMissingMethod()
{
$eventSubscriber = new TestEventSubscriberWithMissingEventMethod();

$this->expectException(\UnexpectedValueException::class);
$this->expectExceptionMessage(
'Cannot infer event name for missing method "Symfony\Component\EventDispatcher\Tests\TestEventSubscriberWithMissingEventMethod::invalidMethod".'
);

$this->dispatcher->addSubscriber($eventSubscriber);
}

public function testImplicitSubscriberWithMissingParameter()
{
$eventSubscriber = new TestEventSubscriberWithMissingParameter();

$this->expectException(\UnexpectedValueException::class);
$this->expectExceptionMessage(
'Cannot infer event name for method "Symfony\Component\EventDispatcher\Tests\TestEventSubscriberWithMissingParameter::invalidMethod". Please add a type-hint for the event calls to the first parameter of the method or configure the event name explicitly.'
);

$this->dispatcher->addSubscriber($eventSubscriber);
}

public function testImplicitSubscriberWithInvalidParameter()
{
$eventSubscriber = new TestEventSubscriberWithInvalidParameter();

$this->expectException(\UnexpectedValueException::class);
$this->expectExceptionMessage(
'Cannot infer event name for method "Symfony\Component\EventDispatcher\Tests\TestEventSubscriberWithInvalidParameter::invalidMethod". Please add a type-hint for the event calls to the first parameter of the method or configure the event name explicitly.'
);

$this->dispatcher->addSubscriber($eventSubscriber);
}

public function testRemoveSubscriber()
{
$eventSubscriber = new TestEventSubscriber();
Expand Down Expand Up @@ -273,6 +316,14 @@ public function testRemoveSubscriberWithMultipleListeners()
$this->assertFalse($this->dispatcher->hasListeners(self::preFoo));
}

public function testRemoveSubscriberWithoutEventName()
{
$eventSubscriber = new TestEventSubscriberWithoutEventName();
$this->dispatcher->addSubscriber($eventSubscriber);
$this->dispatcher->removeSubscriber($eventSubscriber);
$this->assertFalse($this->dispatcher->hasListeners(MockEvent::class));
}

public function testEventReceivesTheDispatcherInstanceAsArgument()
{
$listener = new TestWithDispatcher();
Expand Down Expand Up @@ -484,3 +535,58 @@ public static function getSubscribedEvents()
]];
}
}

class TestEventSubscriberWithoutEventName implements EventSubscriberInterface
{
public static function getSubscribedEvents()
{
return [
'onMockEvent',
['onEarlyMockEvent', 255],
];
}

public function onEarlyMockEvent(MockEvent $event): void
{
}

public function onMockEvent(MockEvent $event): void
{
}
}

class TestEventSubscriberWithMissingEventMethod implements EventSubscriberInterface
{
public static function getSubscribedEvents()
{
return ['invalidMethod'];
}
}

class TestEventSubscriberWithMissingParameter implements EventSubscriberInterface
{
public static function getSubscribedEvents()
{
return ['invalidMethod'];
}

public function invalidMethod(): void
{
}
}

class TestEventSubscriberWithInvalidParameter implements EventSubscriberInterface
{
public static function getSubscribedEvents()
{
return ['invalidMethod'];
}

public function invalidMethod(string $event): void
{
}
}

class MockEvent extends Event
{
}
0