[RTM] Allow to register hooks listeners as tagged services #1094

dmolineus · 2017-09-20T11:14:15Z

Since #376 it's possible to use services for hooks. This is a great step to using services within Contao.

This PR completes this step by allowing to register hooks as services without using the config.php. You don't have to manage your hook registration at two places anymore.

services:
  my.custom.hook_listener:
    class: 'My\Custom\Hook\Listener'
    tags:
      - { name: 'contao.hook', hook: 'parseTemplate', method: 'onParseTemplate'}
      - { name: 'contao.hook', hook: 'initializeSystem', method: 'onInitializeSystem', before: true}

I'm aware of the above mentioned PR discussed it before and this feature wasn't added because of lack of control the position of the hook.

To gain more control of the position it's possible to set the before attribute so that the hook is registered before the legacy hooks. If you really need more control, well, just use the globals.

To keep backward compatibility the registered hooks are added to the $GLOBALS['TL_HOOKS'] array.

aschempp · 2017-09-20T11:37:47Z

I very much like this idea! Even thought we agreed not to "improve" the "old" stuff, this will be helpful a lot. However, I would suggest two changes:

Use priorities for the hooks. We can simply take the existing hook array keys as their internal priority (reversed). This way we will follow Symfony standards.
The setup handling should not be done in the framework class, but using an event listener on the initializeSystem hook (which obviously needs to be registered using the old style ^^). Unfortunately, that would mean initializeSystem hooks are not taggable, but I can't see any other good solution.

leofeyer · 2017-09-20T12:27:30Z

Use priorities for the hooks.

+1

The setup handling should not be done in the framework class

Since it is part of initializing the Contao 3 framework, it would be ok for me (not ideal of course).

dmolineus · 2017-09-20T12:48:45Z

Use priorities for the hooks. We can simply take the existing hook array keys as their internal priority (reversed). This way we will follow Symfony standards.

I thought about it, but wasn't sure how to handle the legacy hooks. To confirm if I got it right:

Given are registered hooks in $GLOBALS['TL_HOOKS'] and I want to add my hook at a specific position:

<?php
$GLOBALS['TL_HOOKS']['parseTemplate'] = [
  ['Foo', 'onParseTemplate'],
  ['Bar', 'onParseTemplate'],
  ['Baz', 'onParseTemplate'],
];

So your suggestion is, that the Foo listener is handled as priority 2, Bar as priority 1 and Baz ans priority 0. To if I want to be at first position I have to use priority 4. Did I understand you correctly?

What I dislike here that you never know how many hooks are registered. Symfony standard doesn't depend on the number of listeners as well. I suggest handle all legacy hooks as priority 0 instead.

Toflar · 2017-09-20T15:29:44Z

Cool, I like the idea, well done @dmolineus!

aschempp · 2017-09-21T06:16:14Z

I would agree with the priority 0, but I see issues here. The legacy order is important, because extensions are loaded after each other and might depend on it. So we need to make sure the legacy order stays the same. The most simple solution will be to array_reverse the old hooks and then use the keys as priorities.

aschempp · 2017-09-21T06:17:16Z

Regarding initialization in the framework class, let's just make sure we agree on this once and for all, because @Toflar used the hook in the new module as controller pull request to merge legacy with new stuff. Everything should go into the framework class then, and that'll make it huge…

dmolineus · 2017-09-21T06:45:44Z

I would agree with the priority 0, but I see issues here. The legacy order is important, because extensions are loaded after each other and might depend on it. So we need to make sure the legacy order stays the same. The most simple solution will be to array_reverse the old hooks and then use the keys as priorities.

I would solve it this way:

Priority > 0 will be added before the legacy hooks depending on the priority
Now the legacy hooks are added in the ordered they are added
Priority = 0 comes after the legacy hooks
Priority < 0 comes after that

Another solution could be to to load the legacy hooks and hooks with priority null for each bundle at the same time. So the order would be:

Priority > 0 will be added before the legacy hooks depending on the priority
Bundle A legacy hooks
Bundle A hook listeners with priority 0
Bundle B legacy hooks
Bundle B hook listeners with priority 0
...
Priority < 0 comes after that

dmolineus · 2017-09-21T06:47:48Z

Regarding initialization in the framework class, let's just make sure we agree on this once and for all, because @Toflar used the hook in the new module as controller pull request to merge legacy with new stuff. Everything should go into the framework class then, and that'll make it huge…

Maybe adding an event which is triggered before the initialization hook would allow to handle such tasks as the module as controller feature or the hook listener registration.

Toflar · 2017-09-21T14:16:56Z

let's just make sure we agree on this once and for all

Agree on what?

leofeyer · 2017-09-21T14:22:58Z

Regarding initialization in the framework class, let's just make sure we agree on this once and for all, because @Toflar used the hook in the new module as controller pull request to merge legacy with new stuff. Everything should go into the framework class then, and that'll make it huge…

He means "agree on not putting everything into the framework class and use listeners/hooks instead". Although this is a special case IMO, because of

Unfortunately, that would mean initializeSystem hooks are not taggable, but I can't see any other good solution.

aschempp · 2017-09-21T19:16:00Z

I would still prefer to call an explicit event listener manually instead of having everything in the framework class…

Regarding the priority 0, I'm not sure if the order of hooks inside one priority is kept when sorting them by priority?

dmolineus · 2017-09-22T08:01:55Z

I reworked the hook registration using the priority attribute. I've chosen this strategy.

Priority > 0 will be added before the legacy hooks depending on the priority
Now the legacy hooks are added in the ordered they are added
Priority = 0 comes after the legacy hooks
Priority < 0 comes after that

Hooks with same priority will keep their internal order.

aschempp · 2017-10-03T08:09:48Z

src/DependencyInjection/Compiler/RegisterHooksPass.php

+            foreach ($tags as $attributes) {
+                $this->guardRequiredAttributesExist($serviceId, $attributes);
+
+                $priority = $attributes['priority'] ?? 0;


You should probably int-cast this for safety reasons.

Done in ec49f63.

aschempp · 2017-10-03T08:11:35Z

src/DependencyInjection/Compiler/RegisterHooksPass.php

+
+        if (count($hooks) > 0) {
+            // Apply priority sorting.
+            krsort($hooks);


are you sure this works? Because it sorts on the key of the hook name, not the priority? Also, you might want to use the \SplPriorityQueue which is exactly meant for this.

Actually I was because I craeted unit tests for it. But I had a closer look. It didn't work. assertEquals does not detect order of keys which I thought it would (and which I was used to do by PHPSpec which I actually prefer).

Fixed it in 802350f. Just used simple arrays as Symfony does the same in the event dispatcher.

aschempp · 2017-10-03T08:16:14Z

src/DependencyInjection/Compiler/RegisterHooksPass.php

+            // Apply priority sorting.
+            krsort($hooks);
+
+            $container->setParameter('contao.hook_listeners', $hooks);


The hooks should not be registered as a parameter that is always available, but we should use dependency injection for it. That means something like $container->getDefinition('contao.framework')->setArgument(6, $hooks).

Implemented in 728799e.

aschempp · 2017-10-03T08:22:09Z

src/Framework/ContaoFramework.php

+            foreach ($config as $hookName => $priorities) {
+                $hooks = [];
+
+                foreach ($priorities as $priority => $listeners) {


I think this can be simplified to something like this:

if (isset($GLOBALS['TL_HOOKS'][$hookName])) { if (isset($priorities[0]) { $priorities[0] = array_merge($GLOBALS['TL_HOOKS'][$hookName], $priorities[0]); } else { $priorities[0] = $GLOBALS['TL_HOOKS'][$hookName]; } } $GLOBALS['TL_HOOKS'][$hookName] = array_merge_recursive($priorities);

No, because array_merge_recursive would merge like this:

$arrayA = ['foo','bar']; $arrayB = ['baz']; // would create [['foo', 'baz'], 'bar] $merged = array_merge_recursive($arrayA, $arrayB);

Toflar · 2017-10-17T12:55:01Z

I would have a use case just now again :) Looking forward to this!

aschempp · 2017-10-17T14:47:38Z

I just looked into this again. Came up with the idea whether it might be clever to actually use a Symfony EventDispatcher for hooks? Meaning that for every hook service in the container, we register a closure that calls the eventDispatcher with a GenericEvent ?

This way we could actually migrate from our hooks to real events. The only disadvantage would be that we can't have priorities mixed with the existing ones. Legacy hooks always come before or after the event dispatcher.

dmolineus · 2017-10-18T09:59:01Z

I just looked into this again. Came up with the idea whether it might be clever to actually use a Symfony EventDispatcher for hooks? Meaning that for every hook service in the container, we register a closure that calls the eventDispatcher with a GenericEvent ?

What would be the benefit of using a GenericEvent instead of named events for it as prepared in #204? (Ingoring the initial work which has to be done to create it).

Toflar · 2017-10-18T10:00:40Z

The only disadvantage would be that we can't have priorities mixed with the existing ones. Legacy hooks always come before or after the event dispatcher.

That's a no go because that's exactly what I would use this PR for.

aschempp · 2017-10-18T10:35:00Z

What would be the benefit of using a GenericEvent instead of named events for it as prepared in #204? (Ingoring the initial work which has to be done to create it).

The difference to me is that you don't need to add event dispatcher code everywhere that hooks are already executed.

But I agree with @Toflar that priority settings is somewhat more important.

aschempp · 2017-10-18T10:41:28Z

src/Framework/ContaoFramework.php

+     */
+    private function buildHookGlobals(): void
+    {
+        foreach ($this->hookListeners as $hookName => $priorities) {


I thought I already mentioned that somewhere, but can't find it. These lines can be much simplified like this:

foreach ($this->hookListeners as $hookName => $priorities) { if (isset($GLOBALS['TL_HOOKS'][$hookName]) && is_array($GLOBALS['TL_HOOKS'][$hookName])) { if (isset($priorities[0]) { $priorities[0] = array_merge($GLOBALS['TL_HOOKS'][$hookName], $priorities[0]); } else { $priorities[0] = $GLOBALS['TL_HOOKS'][$hookName]; } } $GLOBALS['TL_HOOKS'][$hookName] = array_merge_recursive($priorities); }

You did, but it's still not true. array_merge_recursive with one argument will do nothing. Or do you mean array_merge_recursive(...$priorities)?

Sorry, the correct call would be call_user_func_array('array_merge', $priorities)

Okay I added your suggest change in 863f813. Think it's not really simplier, but I don't mind.

aschempp · 2017-10-24T12:19:26Z

src/DependencyInjection/Compiler/RegisterHooksPass.php

+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+/**
+ * Find hook services and store them in an parameter.


… initialized before.

…y > 0 were added.

… necessary to pass the assertion.

Toflar · 2017-10-26T14:38:40Z

So cool, thank you @dmolineus !

aschempp · 2017-11-01T08:22:30Z

❤️

Description ----------- - Commits ------- 09204b73 Upgrade to PHPStan 0.12

leofeyer added the feature label Sep 20, 2017

leofeyer added this to the 4.5.0 milestone Sep 20, 2017

leofeyer changed the title ~~Allow to register hooks listeners as tagged services~~ [WIP] Allow to register hooks listeners as tagged services Sep 20, 2017

leofeyer force-pushed the develop branch from d143e38 to 63ef859 Compare September 22, 2017 07:07

leofeyer force-pushed the develop branch 4 times, most recently from ed0a7e5 to 3dadd44 Compare September 28, 2017 10:39

leofeyer requested a review from aschempp September 29, 2017 13:47

aschempp reviewed Oct 3, 2017

View reviewed changes

leofeyer force-pushed the develop branch from e37e989 to dad087d Compare October 3, 2017 19:03

dmolineus changed the title ~~[WIP] Allow to register hooks listeners as tagged services~~ [RTM] Allow to register hooks listeners as tagged services Oct 5, 2017

leofeyer force-pushed the develop branch from f131963 to e665dae Compare October 17, 2017 07:50

aschempp suggested changes Oct 18, 2017

View reviewed changes

aschempp approved these changes Oct 24, 2017

View reviewed changes

leofeyer assigned dmolineus Oct 26, 2017

dmolineus added 13 commits October 26, 2017 14:04

Implement hook as services registration feature.

1e25137

Workaround: Force buildHookGlobals being called even if framework got…

fd77459

… initialized before.

Add missing return types.

e7f4ec3

Use priorities instead of before flag for hook listeners.

3515924

Fix issue that legacy hooks got lost when only listeners with priorit…

0cc6a83

…y > 0 were added.

Fix broken merging.

22ec21d

Int cast priority.

de29c6c

Don't use assertEquals when comparing array because same order is not…

161bbc0

… necessary to pass the assertion.

Fix sorting of the hooks.

73af006

Pass hook listeners as constructor argument to the contao framework.

2d02e15

Restore changed caused by pulling from upstream develop.

282789c

Remove not required count if statement.

4f7b4fa

Simplify buildHooksGlobals as suggested by @aschempp.

9f549f5

leofeyer force-pushed the feature/tagged-hook-services branch from 863f813 to 9f549f5 Compare October 26, 2017 12:05

Fix the coding style and add a change log entry.

791b1f9

leofeyer merged commit e700e19 into contao:develop Oct 26, 2017

dmolineus deleted the feature/tagged-hook-services branch October 26, 2017 13:48

m-vo mentioned this pull request Dec 14, 2017

Thumbnail-size in Filemanager #1236

Closed

richardhj mentioned this pull request Jul 6, 2018

Allow to register hooks listeners as tagged services isotope/core#1957

Closed

leofeyer added a commit that referenced this pull request Dec 13, 2019

Upgrade to PHPStan 0.12 (see #1094)

0a69b47

Description ----------- - Commits ------- 09204b73 Upgrade to PHPStan 0.12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Allow to register hooks listeners as tagged services #1094

[RTM] Allow to register hooks listeners as tagged services #1094

dmolineus commented Sep 20, 2017 •

edited

Loading

aschempp commented Sep 20, 2017

leofeyer commented Sep 20, 2017

dmolineus commented Sep 20, 2017 •

edited

Loading

Toflar commented Sep 20, 2017

aschempp commented Sep 21, 2017

aschempp commented Sep 21, 2017

dmolineus commented Sep 21, 2017

dmolineus commented Sep 21, 2017

Toflar commented Sep 21, 2017

leofeyer commented Sep 21, 2017

aschempp commented Sep 21, 2017

dmolineus commented Sep 22, 2017

aschempp Oct 3, 2017

dmolineus Oct 4, 2017

aschempp Oct 3, 2017

dmolineus Oct 4, 2017

aschempp Oct 3, 2017

dmolineus Oct 4, 2017

aschempp Oct 3, 2017

dmolineus Oct 4, 2017

Toflar commented Oct 17, 2017

aschempp commented Oct 17, 2017

dmolineus commented Oct 18, 2017

Toflar commented Oct 18, 2017

aschempp commented Oct 18, 2017 •

edited

Loading

aschempp Oct 18, 2017

dmolineus Oct 18, 2017

aschempp Oct 18, 2017

dmolineus Oct 19, 2017 •

edited

Loading

aschempp Oct 24, 2017

Toflar commented Oct 26, 2017

aschempp commented Nov 1, 2017

[RTM] Allow to register hooks listeners as tagged services #1094

[RTM] Allow to register hooks listeners as tagged services #1094

Conversation

dmolineus commented Sep 20, 2017 • edited Loading

aschempp commented Sep 20, 2017

leofeyer commented Sep 20, 2017

dmolineus commented Sep 20, 2017 • edited Loading

Toflar commented Sep 20, 2017

aschempp commented Sep 21, 2017

aschempp commented Sep 21, 2017

dmolineus commented Sep 21, 2017

dmolineus commented Sep 21, 2017

Toflar commented Sep 21, 2017

leofeyer commented Sep 21, 2017

aschempp commented Sep 21, 2017

dmolineus commented Sep 22, 2017

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

Toflar commented Oct 17, 2017

aschempp commented Oct 17, 2017

dmolineus commented Oct 18, 2017

Toflar commented Oct 18, 2017

aschempp commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmolineus Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Oct 26, 2017

aschempp commented Nov 1, 2017

dmolineus commented Sep 20, 2017 •

edited

Loading

dmolineus commented Sep 20, 2017 •

edited

Loading

aschempp commented Oct 18, 2017 •

edited

Loading

dmolineus Oct 19, 2017 •

edited

Loading