[EventDispatcher] Add Drupal EventDispatcher #12521

znerol · 2014-11-19T20:59:41Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	?
Fixed tickets	#12007
License	MIT (relicensing to be approved by three contributors)
Doc PR	-

Drupal forked the EventDispatcher for performance reasons a while ago because the construction of the event dispatcher service turned out to be an order of magnitude faster when listeners are injected into the constructor instead of adding them one-by-one with method calls (see Drupal issue 1972300).

This PR contains that version with Symfony coding-standards applied and short-array syntax removed. Obviously the class name hardly should be DrupalEventDispatcher but I cannot figure out a good name atm. Let's first see whether Symfony is willing to adopt it.

znerol · 2014-11-19T21:05:30Z

Note that #12131 would also help a lot here, regardless of whether this PR will be accepted or not.

unkind · 2014-11-19T21:55:18Z

Why you don't patch existing RegisterListenersPass and ContainerAwareEventDispatcher instead?

znerol · 2014-11-20T08:22:17Z

Because that breaks BC and thus cannot be accepted in Symfony, see #12019

javiereguiluz · 2014-11-20T08:25:00Z

src/Symfony/Component/EventDispatcher/DependencyInjection/DrupalRegisterListenersPass.php

+ * file that was distributed with this source code.
+ */
+
+namespace Drupal\Core\DependencyInjection\Compiler;


Shouldn't this namespace be changed to namespace Symfony\Component\EventDispatcher\DependencyInjection;?

henrikbjorn · 2014-11-20T08:26:57Z

Seems like it should be possible to add an optional constructor argument to the current EventDispatcher instead of doing a new class.

henrikbjorn · 2014-11-20T08:27:31Z

src/Symfony/Component/EventDispatcher/DrupalEventDispatcher.php

+ *   </dd>
+ * </dl>
+ */
+class ContainerAwareEventDispatcher implements EventDispatcherInterface


The class name does not correspond with the filename

henrikbjorn · 2014-11-20T08:28:57Z

The idea is great, but this lacks tests and have clearly not been run after the move here, as classname and filenames does not correspond.

javiereguiluz · 2014-11-20T08:31:50Z

src/Symfony/Component/EventDispatcher/DrupalEventDispatcher.php

+                        $definition['callable'] = array($this->container->get($definition['service'][0]), $definition['service'][1]);
+                    }
+
+                    $definition['callable']($event, $event_name, $this);


If I'm right, this dereferencing syntax is only available in PHP 5.4+, so we couldn't use it for Symfony.

znerol · 2014-11-20T08:32:39Z

Indeed this was just a copy paste with some cleanup which I did mainly for testing the waters. Regarding tests, see #12131.

javiereguiluz · 2014-11-20T08:34:12Z

src/Symfony/Component/EventDispatcher/DrupalEventDispatcher.php

+    /**
+     * {@inheritdoc}
+     */
+    public function dispatch($event_name, Event $event = NULL)


I know it's a minor thing, but in Symfony we usually put null in lowercase.

Note that the PHP-CS-Fixer tool will fix this automatically

javiereguiluz · 2014-11-20T08:50:17Z

@znerol thanks for your PR. As you may now, in Symfony we have a bot that checks if the submitted code complies with the code syntax standards used in the project.

Apparently, your PR needs some minor tweaks to comply with those standards. You can find a patch to fix them all at http://fabbot.io/report/symfony/symfony/12521/61864b082c705f55501bd7ef33e457d2df80656e

Please tell me if you need any help with that.

znerol · 2014-11-20T11:46:37Z

If I'm right, this dereferencing syntax is only available in PHP 5.4+, so we couldn't use it for Symfony.

We actually wondered about that also, but it seems that this has been introduced in 5.3. The following test passes on 3v4l with the relevant versions:

<?php
$f = function ($x) {echo $x;};
$a = array('f' => $f);
$a['f']('hi!');

javiereguiluz · 2014-11-20T12:10:11Z

@znerol you are right! I'm sorry about my comment because the syntax is indeed valid for PHP 5.3.

fabpot · 2014-11-20T12:16:47Z

@znerol #12131 has just been merged in 2.6.

fabpot · 2014-11-20T12:35:11Z

So, the question now is: can this class replace the existing one in Symfony 3.0? If yes, we then need to deprecate the current one in favor of this one.

znerol · 2014-11-20T13:00:49Z

There is a subtle change in behavior when using this class. This has been pointed out by @stof in #12069:

adding all listener classes as container resources is likely to have an impact in dev, as it means that the container will need to be rebuilt each time you change the source code of your listener.

With the ContainerAwareEventDispatcher it is possible to set the priority of a listener dynamically (i.e. on every request), this is not possible with CompiledEventDispatcher. I have no idea whether or not this was intentionally designed like that.

fabpot · 2014-11-20T13:05:24Z

@znerol I don't think there is a practical use case of this drawback. So, even if I can see the BC break here, I don't think it makes sense to support it. IMO, if the "limitation" is well documented, that's more than enough.

znerol · 2014-11-20T13:17:16Z

Ok, then the following things need to be done:

Add tests for the event dispatcher based on Extract an AbstractEventDispatcherTest from EventDispatcherTest #12131
Fix the TODO in the compiler pass
Add tests for the compiler pass
Get permission of @chx and @dawehner for the license change (GPL -> MIT).
Deprecate ContainerAwareEventDispatcher (should probably be in a separate pull request?) and maybe also rewrite the docs in CompiledEventDispatcher.

Regarding the compiler pass, is it okay to simply add the array of listeners to the list of constructor arguments?

GromNaN · 2014-11-20T13:38:22Z

src/Symfony/Component/EventDispatcher/CompiledEventDispatcher.php

+    /**
+     * Whether listeners need to be sorted prior to dispatch, keyed by event name.
+     *
+     * @var TRUE[]


The type doc should be @var array

stof · 2014-11-21T04:08:07Z

The compiler pass to initialize the dispatcher should be provided as well (otherwise we won't be able to use it by default).

And I don't see how you will handle the lazy loading of event subscriber in your listener. There is no logic to handle this in your code.
And note that resolving events in the compiler pass is precisely was caused previous attempts to be rejected, because it breaks the debug mode (by forcing to clear the cache manually even in debug mode when changing the list of events, which is bad for the DX as Symfony developers are used of having automatic cache reloading in dev)

stof · 2014-11-21T04:08:59Z

and registering subscribers in a non-lazy way is not an option either given that all Symfony listeners are defined as subcribers to be easier to reuse them in other frameworks like Silex

znerol · 2014-11-21T07:35:32Z

The compiler pass to initialize the dispatcher should be provided as well

It is there

And I don't see how you will handle the lazy loading of event subscriber in your listener.

If you mean $container->get('some-service'); it is there, if you are referring to something different, please explain.

And note that resolving events in the compiler pass is precisely was caused previous attempts to be rejected.

The intention of this PR is to provide an alternative implementation, not to replace the existing ContainerAwareEventDispatcher.

znerol · 2014-11-22T12:07:54Z

Travis failures for PHP 5.3 are the same as for the current 2.7 branch, no test failures in the event dispatcher component.

chx · 2014-11-23T21:58:39Z

You have my permission for this. I am definitely not happy with MIT licensing my code but let's do it.

znerol · 2015-02-18T18:23:40Z

@stof / @fabpot ping

znerol · 2015-03-03T07:34:04Z

Symfony 2.7 is about to enter feature-freeze. We should decide soon on whether or not this gets in.

nicolas-grekas · 2016-12-12T12:47:20Z

I'd be in favor of closing this one and work on #20875 instead.

nicolas-grekas · 2016-12-19T13:01:22Z

More generic solution posted in #20953, based on closure injection + compile time resolution of subscribers.
Could you please Drupal guys have a look at it and see if it looks good for Drupal also?

nicolas-grekas · 2017-01-06T14:43:56Z

Closed in favor of #20953

…t type (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI][EventDispatcher] Add & wire closure-proxy argument type | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12521, #12007, #20039 | License | MIT | Doc PR | - By resolving event subscribers at compile time, then wrapping listeners in a closure, we get laziness almost for free. This should solve/replaced all the above linked issues. (WIP because tests are missing) Commits ------- ecdf857 [DI][EventDispatcher] Add & wire closure-proxy argument type

javiereguiluz reviewed Nov 20, 2014
View reviewed changes

henrikbjorn reviewed Nov 20, 2014
View reviewed changes

javiereguiluz reviewed Nov 20, 2014
View reviewed changes

GromNaN reviewed Nov 20, 2014
View reviewed changes

znerol force-pushed the drupal-event-dispatcher branch from 6237aeb to b91caf1 Compare November 21, 2014 10:38

znerol added 11 commits January 18, 2015 10:52

Use camel-case for variable names

002cf8b

Add test coverage for CompiledRegisterListenersPass

2e39ed6

Complete test coverage for CompiledEventDispatcher

a2cd250

Fix colliding mock subscriber classes

60845da

PHP 5.3 does not support callables in array($object, $method) form

3a4f177

Fix coding style

273a397

Fix docs on compiled event dispatcher

02d84f7

Use private visibility for instance variables

0150640

Fix codestyle as suggested by @stof

d0f986d

Use string keys for service definition

0bee27f

Replicate symfony#13293

9a9b31c

znerol force-pushed the drupal-event-dispatcher branch from cd1d0bf to 9a9b31c Compare January 18, 2015 10:11

Fix code style

b4f56ab

unkind mentioned this pull request Jul 8, 2015

[DependencyInjection] Inlining of large parameters #15203

Closed

javiereguiluz added the Feature label Mar 3, 2016

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas mentioned this pull request Dec 12, 2016

[DI] Add "lazy collection" type #20875

Closed

nicolas-grekas mentioned this pull request Dec 16, 2016

[DI][EventDispatcher] Add & wire closure-proxy argument type #20953

Merged

nicolas-grekas closed this Jan 6, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Koc mentioned this pull request Apr 22, 2018

Compiled EventSubscriber for possible performance optimization #26993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EventDispatcher] Add Drupal EventDispatcher #12521

[EventDispatcher] Add Drupal EventDispatcher #12521

znerol commented Nov 19, 2014

znerol commented Nov 19, 2014

unkind commented Nov 19, 2014

znerol commented Nov 20, 2014

javiereguiluz Nov 20, 2014

henrikbjorn commented Nov 20, 2014

henrikbjorn Nov 20, 2014

henrikbjorn commented Nov 20, 2014

javiereguiluz Nov 20, 2014

znerol commented Nov 20, 2014

javiereguiluz Nov 20, 2014

stof Nov 20, 2014

javiereguiluz commented Nov 20, 2014

znerol commented Nov 20, 2014

javiereguiluz commented Nov 20, 2014

fabpot commented Nov 20, 2014

fabpot commented Nov 20, 2014

znerol commented Nov 20, 2014

fabpot commented Nov 20, 2014

znerol commented Nov 20, 2014

GromNaN Nov 20, 2014

stof commented Nov 21, 2014

stof commented Nov 21, 2014

znerol commented Nov 21, 2014

znerol commented Nov 22, 2014

chx commented Nov 23, 2014

znerol commented Feb 18, 2015

znerol commented Mar 3, 2015

nicolas-grekas commented Dec 12, 2016

nicolas-grekas commented Dec 19, 2016

nicolas-grekas commented Jan 6, 2017

[EventDispatcher] Add Drupal EventDispatcher #12521

[EventDispatcher] Add Drupal EventDispatcher #12521

Conversation

znerol commented Nov 19, 2014

znerol commented Nov 19, 2014

unkind commented Nov 19, 2014

znerol commented Nov 20, 2014

javiereguiluz Nov 20, 2014

Choose a reason for hiding this comment

henrikbjorn commented Nov 20, 2014

henrikbjorn Nov 20, 2014

Choose a reason for hiding this comment

henrikbjorn commented Nov 20, 2014

javiereguiluz Nov 20, 2014

Choose a reason for hiding this comment

znerol commented Nov 20, 2014

javiereguiluz Nov 20, 2014

Choose a reason for hiding this comment

stof Nov 20, 2014

Choose a reason for hiding this comment

javiereguiluz commented Nov 20, 2014

znerol commented Nov 20, 2014

javiereguiluz commented Nov 20, 2014

fabpot commented Nov 20, 2014

fabpot commented Nov 20, 2014

znerol commented Nov 20, 2014

fabpot commented Nov 20, 2014

znerol commented Nov 20, 2014

GromNaN Nov 20, 2014

Choose a reason for hiding this comment

stof commented Nov 21, 2014

stof commented Nov 21, 2014

znerol commented Nov 21, 2014

znerol commented Nov 22, 2014

chx commented Nov 23, 2014

znerol commented Feb 18, 2015

znerol commented Mar 3, 2015

nicolas-grekas commented Dec 12, 2016

nicolas-grekas commented Dec 19, 2016

nicolas-grekas commented Jan 6, 2017