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

HookDispatcher issue #10412

Open
sarjon opened this Issue Sep 12, 2018 · 27 comments

Comments

@sarjon
Member

sarjon commented Sep 12, 2018

I've noticed that even though hook is dispatched somewhere in the core, it not executed on module. Here's an example:

This is how hook is dispatched in grid definition factory:

// PrestaShop\PrestaShop\Core\Grid\Definition\Factory\AbstractGridDefinitionFactory

$this->hookDispatcher->dispatchWithParameters('action' . $definition->getId() . 'GridDefinitionModifier', [
    'definition' => $definition,
]);

However, in module hook is never executed. But if I dispatch hook using legacy Hook like this:

// PrestaShop\PrestaShop\Core\Grid\Definition\Factory\AbstractGridDefinitionFactory

\Hook::exec('action' . $definition->getId() . 'GridDefinitionModifier', [
    'definition' => $definition,
]);

This way module hook is executed and everything works good.

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 12, 2018

@mickaelandrieu any ideas? 🤔

do we actually need to depend on Symfony's event dispatcher in PrestaShop\PrestaShop\Adapter\Hook\HookDispatcher? wouldnt it be better to implement HookDispatcherInterface using legacy Hook?

at the moment is seems that HookDispatcher === EventDispatcher and Hook === Event? shouldnt hook dispatcher be specific to PrestaShop with it's own implementation better?

@khouloudbelguith khouloudbelguith added the CO label Sep 13, 2018

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 19, 2018

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 19, 2018

It's not that critical I've expected: as these hooks are new and not registered in the database until you register a module on it they won't be displayed.

The best we can do here from a Developer eXperience point of view is to register them in the file hook.xml

@matks matks added the migration label Sep 24, 2018

@matks matks changed the title from HookDispatcher issue to [SF Migration] HookDispatcher issue Sep 24, 2018

@matks matks added this to To do in Symfony Migration via automation Sep 24, 2018

@matks matks changed the title from [SF Migration] HookDispatcher issue to HookDispatcher issue Sep 24, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 24, 2018

ok, basically the problem is that legacy Hook:exec() does not care about case sensitivity, for example: actionProductUpdate is the same as actionproductupdate or ACTIONPRODUCTUPDATE, but the LegacyHookSubscriber which is responsible for dispatching legacy hooks in new code does.

I think we should fix LegacyHookSubscriber to be case-insensitive so we keep same behavior across legacy and new code, @matks wdyt?

@matks

This comment has been minimized.

Contributor

matks commented Sep 24, 2018

@sarjon Sadly you are right, this is required to ensure modules compatibility. But we'll change this in prestashop 1.8.

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 24, 2018

its totally okay to change it in 1.8, but for now in 1.7, i think it should have same behavior as in legacy.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 24, 2018

I think we should fix LegacyHookSubscriber to be case-insensitive so we keep same behavior across legacy and new code, @matks wdyt?

I repeat again (xD), there is nothing we can do in the LegacyHookSubscriber.

And for what it means, I'm in favor of doing the opposite: make Hooks case sensitive instead by deprecating it now and removing it in 1.8.x.

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 24, 2018

make Hooks case sensitive

i guess it can only be done in next major as this change could potentially affect a lot of modules, right?

but for now, it seems like a very bad experience to have "same" hook system that behaves differently between legacy and migrated pages. :/

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 24, 2018

but for now, it seems like a very bad experience to have "same" hook system that behaves differently between legacy and migrated pages. :/

Nothing we can manage by documenting it and also deprecate and discourage using non-sensitive calls to hooks.

@matks matks moved this from To do to In progress in Symfony Migration Sep 25, 2018

@matks

This comment has been minimized.

Contributor

matks commented Sep 26, 2018

@mickaelandrieu Why do you say:

I repeat again (xD), there is nothing we can do in the LegacyHookSubscriber.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 28, 2018

The LegacyHookSubscriber register listeners to the HookDispatcher according to the hooks already registered in the database, it's like doing:

for each hook in ps_hooks
    $hookDispatcher->addListener('hookname', callable);

So no listener will "listen" to "HooKName", "hook_name" or "hookName".

We won't pollute our database with all possible syntaxes for a hook nor register thousand more listeners to the Hook Dispatcher because some developers are just too lazy to read the docs or "most of the time" the list of hooks collected in the debugger.

If you ask me it's a bug that executing a hook is not case sensitive 👼

@matks

This comment has been minimized.

Contributor

matks commented Sep 28, 2018

And how does the ps_hooks database gets populated ?

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 28, 2018

how about

foreach strtolower(hook) in ps_hooks
    $hookDispatcher->addListener('hookname', callable);

as in legacy hook it does that, so maybe we can have same behavior everywhere?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 28, 2018

@sarjon doing that in 1.7 introduces a BC, but yeah it's a workaround. IMHO, 2 things:

  • we need to remove the use of the database here;
  • we need to check what is inserted in hooks table;

But eh! I trust you guys to find something, It's time for me to leave far far away from the sea...

https://www.youtube.com/watch?v=89cJoRH45vM

@matks

This comment has been minimized.

Contributor

matks commented Sep 28, 2018

If we combine @sarjon suggestion with an additionnal strtolower in $this->hookDispatcher->dispatchWithParameters() we should be able to make sure every hook name get lowercased 🤔

@matks matks added this to To do in PrestaShop 1.7.5 via automation Oct 1, 2018

@matks

This comment has been minimized.

Contributor

matks commented Oct 1, 2018

Has to be done for 1.7.5

@matks

This comment has been minimized.

Contributor

matks commented Oct 2, 2018

we need to remove the use of the database here;

Do you suggest we use the HookRegistry SF service instead ?

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 2, 2018

why do we want to use 2 different systems to dispatch hooks in the first place? :/ can't we keep Hook:exec() and rework it to use events in the next major?

@matks

This comment has been minimized.

Contributor

matks commented Oct 2, 2018

So, after diving into the code of core class Hook, and adapter classes / SF services LegacyHookSubscriber and HookDispatcher, one possible plan is:

  • modify HookDispatcher to normalize dispatched hook names so that one_HooK_name becomes onehookname
  • modify core class Hook to normalize registered hook names too
  • so hook names in database and dispatched hook names should be identical everywhere

Possible BC break: if previously there was 1 hook iAmHookOne and iamhookone, they will be considered duplicates now whereas there were 2 different hooks before.


@sarjon I will try to find out the reasons why the 2nd hook system has been introduced in SF. However I guess that the 2nd system is a better implementation of hooking process as it relies on SF events dispatch and benefits from the strong and reliable implementation beneath. For example it allows to introduces middlewares, have the hook list in the SF debug bar, it probably could allow us to perform hooks after the HTTP response has been sent (this is a native feature of SF events dispatcher) which can improve 1 page optimization ...

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 2, 2018

2nd system is a better implementation of hooking process as it relies on SF events dispatch and benefits from the strong and reliable implementation beneath

even if thats true, having 2 systems dispatching hooks differently is bad, there should be one either using events or legacy. :/

have the hook list in the SF debug bar

hook registration is performed in legacy class, so thats not a case.

which can improve 1 page optimization

not sure if that works on apache as most of the sites are using it. :/

@matks matks assigned matks and sarjon and unassigned matks Oct 5, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 10, 2018

@matks i tried to find a simple fix for it, but without any luck. 😢 it seems to me that new hook system is too complicated and here's why:

  1. there are 2 hook dispatchers:
    • PrestaShop\PrestaShop\Adapter\Hook\HookDispatcher - its in Adapter namespace but does not have any legacy or adapter dependencies. It extends Symfony's event dispathcer to modify doDispatch implementation. Also, this dispatcher is used to register ~230 events listeners on every page load and it's on clean PrestaShop installation. I can image there could be 500+ or even 1000+ listeners on every page load on actual shop, even though a few listeners (hooks) are needed in most cases. :/
    • PrestaShop\PrestaShop\Core\Hook\HookDispatcher - another hook dispatcher that depends on HookDispatcher that's in Adapter namespace. Basically, every call is forwarded to adapter hook dispatcher.
  2. PrestaShop\PrestaShop\Core\Hook\HookDispatcherInterface - should be a hook dispatcher interface, but it actually extends Symfony\Component\EventDispatcher\EventDispatcherInterface just to make it work with Symfony's event dispatcher. So it adds API like addSubscriber, hasListeners and etc. to HookDispacther, what theses subscribers and listeners have to do with hooks? 🤔

This is what i came up with:

  1. Stop mixing legacy hooks with Symfony events and keep HookDispatcherInterface standalone and implement single HookDispatcher adapter that fowards hook dispatching to legacy Hook::exec().
  2. If we want to encourage use of Symfony evenst, we could use them in new components instead of Hooks.
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 30, 2018

Stop mixing legacy hooks with Symfony events and keep HookDispatcherInterface standalone and implement single HookDispatcher adapter that forward hooks dispatching to legacy Hook::exec()

In the end, Hooks will be Symfony events, we have "no" interest at all at a contract on hooks that does not relies on the Symfony Event Dispatcher component. The final end is to use the current event dispatcher to dispatch both Symfony events and hooks (let's say "PrestaShop events").

For now and I hope you are already aware of it but we can't have only one implementation:

  • Front Office is still using PrestaShop Framework
  • Back Office migration is not finished

From an architectural point of view, an Adapter doesn't always mean "must use Legacy class in it", it's only about a transitional state.

Implementation A (current, PrestaShop Framework-based) <===> Adapter <===> Implementation B (Symfony-based)

Also, this dispatcher is used to register ~230 events listeners on every page load and it's on clean PrestaShop installation. I can imagine there could be 500+ or even 1000+ listeners on every page load on an actual shop, even though a few listeners (hooks) are needed in most cases. :/

First, let's measure it, please provides a Blackfire profile. Then, afaik "in the end" we will have to deal with as many events as hooks available in PrestaShop. So, it's not an issue but a constraint we need to manage (and we already do, Hooks have a big impact on the performance).

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 30, 2018

Hello @marionf I'm not sure Regression applies, or we could consider it's a regression from 1.6.1.x?

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 30, 2018

We talked about this during the sprint planning and from what I understood it's a regression compared to what we had in legacy and it needs to be fixed for 1.7.5
Tell me if I missunderstood @matks

@matks

This comment has been minimized.

Contributor

matks commented Nov 7, 2018

The regression is that SF hook system is case-sensitive and legacy hook system is case-insensitive. So what could be done before in legacy pages cannot be done anymore in SF pages. It looks like a regression to me.

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 7, 2018

Is it a regression or a BC break? maybe we want those hooks to be case sensitive, why not introduce this modification in the 1.7 instead of 1.8
Or it could be a "progressive" BC break, what I mean is if legacy hooks remain case insensitive it won't break, but new ones (using the SF Hook service) can be case sensitive, this prevents new hook to be poorly used case insensitively

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Nov 19, 2018

Is it a regression or a BC break? maybe we want those hooks to be case sensitive, why not introduce this modification in the 1.7 instead of 1.8

Even if it was not intended, I want this to be case sensitive. So if we "fix" this behavior now, the same people will complain when we will change it again in 1.8: is it worth now we have this behavior since 1.7.0?

Let's sum up the situation here:

  1. In 1.6.1.x hooks were case insensitive: note this behavior was probably not intended as it is documented nowhere and not described in docs (I've found information in an issue);
  2. In 1.7.0.x, intended or not the hooks became case sensitive. This problem have nothing to do with how we collect information for the profiler (as introduced by @sarjon first) but to the new system that loop into the hooks database to retrieve and create event listeners according to the hooks: if no listener is created, the execution of the hook won't be done;
  3. We are talking about introducing back a feature that was not described in 1.6.1.x but looks like a side effect of a wrong implementation;
  4. From what I see, no one is impacted by this "issue" => https://github.com/PrestaShop/PrestaShop/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+hooks
  5. As an architect I think it is the right behavior: more I'd like to see an error thrown if someone try to register to the "wrong" hook, but this is not possible right now because of BC on Front Office.

About the proposals

foreach strtolower(hook) in ps_hooks
    $hookDispatcher->addListener('hookname', callable);

Nope, because this will also lowercase Hooks names in the profiler: no only we dispatch the wrongs hooks but more, we will teach to people the wrong names. Totally a LOSE/LOSE situation.

If we combine @sarjon suggestion with an additionnal strtolower in $this->hookDispatcher->dispatchWithParameters() we should be able to make sure every hook name get lowercased

If a solution is possible, it can be done in the adapter only: updating the Hook class is really risky.

Possible BC break: if previously there was 1 hook iAmHookOne and iamhookone, they will be considered duplicates now whereas there were 2 different hooks before.

We are discussing a BC break to introduce back a feature we don't want. Honestly I don't think this BC break will impact shops (how unlucky we will be if it does :trollface: ).

As a conclusion, I can understand what @sarjon is asking here: he wants Hooks system back as in 1.6.
That's an honest request, but what we want in the end is to remove Hooks and replace them by Symfony events and they don't work the same way at all.

In hooks, this is the event dispatcher (ie Hook::exec()) which ask to the modules (event listeners) if someone is interested. From an architectural pov, I guess it's a Publisher/Subscriber system.

In Event Dispatcher Component, the events listeners are registered to the event dispatcher and only the registered listeners on the right events are notified: note that in the end, and for obvious reasons, it's far more performant :) It follows the Event Dispatcher/Emitter system.

In hooks, EVERY module is checked to see if the right function exists and then called, in Symfony events only the right callbacks on the event dispatched are called.

Conclusion

This is the direction we want to take because Hooks are not performant at all and this is why the core team asks for Blackfire report everytime someone want to add one. With the new system we could dispatch 100 000 events and don't see any impacts in the end, once the modules will be allowed to register event listeners to PrestaShop events.

More about it => https://hackernoon.com/observer-vs-pub-sub-pattern-50d3b27f838c?gi=a12d0ab61b2c

Ping @eternoendless, we need a decision here 👍

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 19, 2018

I fully support @mickaelandrieu on this one ^^
I agree with the idea that hook are case insensitive in 1.6 is very likely an unwanted side effect, so we shouldn't try to stick to it so badly
Yes it can be considered a BC break, and yes we have really cold feet on BC breaks

But I think this one is ok and should be accepted (compared to ProductLazyArray it certainly has less impact)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment