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

Introducing HookDispatcherInterface #9437

Merged
merged 14 commits into from Aug 22, 2018

Conversation

@sarjon
Member

sarjon commented Aug 12, 2018

Questions Answers
Branch? develop
Description? Implements new minimal hook dispatcher that might deprecate currently used PrestaShopBundle\Service\Hook\HookDispatcher.
Type? improvement
Category? CO
BC breaks? no
Deprecations? yes
Fixed ticket? n/a
How to test? Tests must pass, and Hook debug toolbar should still display results

What have been done here?

  • Use HookDispatcherInterface everywhere we used HookDispatcher.
  • Deprecated PrestaShopBundle\Service\Hook\HookDispatcher in favor of PrestaShop\PrestaShop\Adapter\Hook\HookDispatcher => more compliant with PrestaShopBundle responsibilities;
  • Introduced PrestaShop\PrestaShop\Core\Hook\HookDispatcher and use it everywhere in Core, we keep the Adapter available until 1.8 => compliant with our migration strategy;
  • Updated NullDispatcher and made him implements HookDispatcherInterface => some technical debt we will address in another contribution;
  • we're still backward-compatible with the Event Dispatcher component! HookDispatcherInterface extends EventDispatcherInterface from Symfony and we rely on EventDispatcher class (even though we don't use the registered one).

Why do we do that?

This is a first step to migrate Hook management to Symfony Event Dispatcher component: for now Hook system is richer on dispatch features, but weaker in "listeners" control. At some point, we will need to decide what to do with Hooks and how to make them more powerful and performant 👍

Rely on interfaces give us time to work on multiples implementations without introduces any breaks.

Next step(s) ?

What we'd like to to is to make the HookDispatcher used as a replacement of the Event Dispatcher so we could dispatch events, regardless these are hooks (PrestaShop events) or Symfony events with the same class. For now the Event Dispatcher is used for Symfony events, and the HookDispacher for Hooks: but now they share a common interface 👍 .

This is something (Make hooks dispatched as Symfony events or Symfony events dispatched as "Hooks") we could do in 1.7 to replace the old Hook system in the next major. For that we also need to use the even dispatcher in Front office.


This change is Reviewable

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 12, 2018

Member

@mickaelandrieu wdyt about this?

Member

sarjon commented Aug 12, 2018

@mickaelandrieu wdyt about this?

/**
* {@inheritdoc}
*/
public function dispatchRendering(HookInterface $hook)

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 13, 2018

Contributor

Should be called ->dispatch too. But check for class interface or capability to use render or not.
If we change the hook capability, we will need to change the method name everywhere, when the first aim is to dispatch a hook. No?
Plus, if you stay with dispatchRendering method, the parameter isn't the right one, must be RenderedHookInterface, same for parent interface and doc block :)

@PierreRambaud

PierreRambaud Aug 13, 2018

Contributor

Should be called ->dispatch too. But check for class interface or capability to use render or not.
If we change the hook capability, we will need to change the method name everywhere, when the first aim is to dispatch a hook. No?
Plus, if you stay with dispatchRendering method, the parameter isn't the right one, must be RenderedHookInterface, same for parent interface and doc block :)

This comment has been minimized.

@sarjon

sarjon Aug 13, 2018

Member

@PierreRambaud thanks for your comment! :)

However, Im not sure if I get it correctly. Why do you think parameter is wrong for dispatchRendering? I think that both types of hooks have same structure (name and parameters) despite whether it's a display or an action hook.

The main difference between dispatch() and dispatchRendering() is that you dont expect anything to be returned from dispatch() (right?) but you always know that dispatchRendering will return RenderedHookInterface.

@sarjon

sarjon Aug 13, 2018

Member

@PierreRambaud thanks for your comment! :)

However, Im not sure if I get it correctly. Why do you think parameter is wrong for dispatchRendering? I think that both types of hooks have same structure (name and parameters) despite whether it's a display or an action hook.

The main difference between dispatch() and dispatchRendering() is that you dont expect anything to be returned from dispatch() (right?) but you always know that dispatchRendering will return RenderedHookInterface.

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 13, 2018

Contributor

I just think like for Symfony and Response for example.
If you dispatch a Response, you don't care if it's a JsonResponse, a BinaryFileResponse or a RedirectResponse. These three classes haven't the same capabilities but are called with the same method =)

@PierreRambaud

PierreRambaud Aug 13, 2018

Contributor

I just think like for Symfony and Response for example.
If you dispatch a Response, you don't care if it's a JsonResponse, a BinaryFileResponse or a RedirectResponse. These three classes haven't the same capabilities but are called with the same method =)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

As a note, we can't use dispatch() as it will conflict with EventDispatcherInterface.

For now, we want to keep the compatibility with the Event Dispatcher component, so we MUST NOT override this function signature.

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

As a note, we can't use dispatch() as it will conflict with EventDispatcherInterface.

For now, we want to keep the compatibility with the Event Dispatcher component, so we MUST NOT override this function signature.

@mickaelandrieu

Some extra work to do but I'm ok with it: good job @sarjon

@mickaelandrieu mickaelandrieu added the wip label Aug 16, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

Some work to do here: remove every direct call to dispatch as for now this method can't be part of the interface named HookDispatcherInterface, I'll do it

Contributor

mickaelandrieu commented Aug 16, 2018

Some work to do here: remove every direct call to dispatch as for now this method can't be part of the interface named HookDispatcherInterface, I'll do it

@mickaelandrieu mickaelandrieu self-assigned this Aug 17, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

I'll finish this one ;)

Contributor

mickaelandrieu commented Aug 17, 2018

I'll finish this one ;)

*/
public function dispatchHook(HookInterface $hook)
{
$this->hookDispatcherAdapter->dispatchForParameters(

This comment has been minimized.

@sarjon

sarjon Aug 17, 2018

Member

hmm, now that i think about it, maybe we actually have to return results even though this is not Rendering hook?

For example, we have a hook paymentOptions, it does not render anything, but rather returns PaymentOption[]. There are more hooks like that.

@sarjon

sarjon Aug 17, 2018

Member

hmm, now that i think about it, maybe we actually have to return results even though this is not Rendering hook?

For example, we have a hook paymentOptions, it does not render anything, but rather returns PaymentOption[]. There are more hooks like that.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

so this should be

return $this->hookDispatcherAdapter->dispatchForParameters()...

instead?

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

so this should be

return $this->hookDispatcherAdapter->dispatchForParameters()...

instead?

This comment has been minimized.

@sarjon

sarjon Aug 17, 2018

Member

yes, as we now have RenderedHookInterface we could return ExecutedHookInterface with getData() which returns executed hook's data if any?

@sarjon

sarjon Aug 17, 2018

Member

yes, as we now have RenderedHookInterface we could return ExecutedHookInterface with getData() which returns executed hook's data if any?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

why not, let me think about it: I'm in favor of typed hook return, but I may not want to introduce it "now".

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

why not, let me think about it: I'm in favor of typed hook return, but I may not want to introduce it "now".

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

also, I was thinking about further refactoring:

ModuleManagementEvent should be splitted into:

  • AbstractModuleEvent
  • InstallModuleEvent
  • UninstallModuleEvent
  • ... Event

And they have to extend HookEvent instead of Event so we could call dispatchHook(AbstractModuleEvent ...) in ModuleManager: wdyt?

Same for ModuleZipManagementEvent (nice name that means nothing :p )

@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

also, I was thinking about further refactoring:

ModuleManagementEvent should be splitted into:

  • AbstractModuleEvent
  • InstallModuleEvent
  • UninstallModuleEvent
  • ... Event

And they have to extend HookEvent instead of Event so we could call dispatchHook(AbstractModuleEvent ...) in ModuleManager: wdyt?

Same for ModuleZipManagementEvent (nice name that means nothing :p )

This comment has been minimized.

@sarjon

sarjon Aug 17, 2018

Member

hmm, i guess it makes sense to have different events for actually different events (install, uninstall & etc).

@sarjon

sarjon Aug 17, 2018

Member

hmm, i guess it makes sense to have different events for actually different events (install, uninstall & etc).

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 17, 2018

Member

I'll finish this one ;)

thanks! :)

Member

sarjon commented Aug 17, 2018

I'll finish this one ;)

thanks! :)

@mickaelandrieu mickaelandrieu changed the title from Implement new hook dispatcher to Introducing HookDispatcherInterface Aug 17, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

ping @PierreRambaud / @Quetzacoalt91 wdyt: go, no go?

Contributor

mickaelandrieu commented Aug 21, 2018

ping @PierreRambaud / @Quetzacoalt91 wdyt: go, no go?

@PierreRambaud

Reviewed 5 of 8 files at r1, 29 of 29 files at r9.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @sarjon, @PierreRambaud, and @mickaelandrieu)


src/Adapter/Hook/HookDispatcher.php, line 55 at r9 (raw file):

     * @throws \Exception If the Event is not HookEvent or a subclass.
     */
    public function dispatch($eventName, Event $event = null)

Should be HookEvent instead Event. And you will be able to remove the test with instanceof


src/Adapter/Hook/HookDispatcher.php, line 86 at r9 (raw file):

     * @throws \Exception If the Event is not HookEvent or a subclass.
     */
    public function dispatchMultiple(array $eventNames, array $eventParameters)

you can make $eventParameters optional


src/Adapter/Hook/HookDispatcher.php, line 131 at r9 (raw file):

     * @param array $parameters Hook parameters
     * @return Event The event that has been passed to each listener.
     * @throws \Exception

use namespace

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

Should be HookEvent instead Event. And you will be able to remove the test with instanceof

this is a BC, we can't do that.

you can make $eventParameters optional

this is a BC, we can't do that.

use namespace

hey, I can actually do that 👍 ... but Event class namespace is already included in the file header: what do you mean?

I can't alter any behavior of the Adapter as it's a copy of the Service, for instance if you wanted to replace HookEvent by Event I can do it as the "contract" become more open (can't break existing). Making $eventParameters optional is ok from a "contract" point of view, but what will happens if at some point the contract is not respected and someone assume the $eventParameters exists? It's risky.

Contributor

mickaelandrieu commented Aug 21, 2018

Should be HookEvent instead Event. And you will be able to remove the test with instanceof

this is a BC, we can't do that.

you can make $eventParameters optional

this is a BC, we can't do that.

use namespace

hey, I can actually do that 👍 ... but Event class namespace is already included in the file header: what do you mean?

I can't alter any behavior of the Adapter as it's a copy of the Service, for instance if you wanted to replace HookEvent by Event I can do it as the "contract" become more open (can't break existing). Making $eventParameters optional is ok from a "contract" point of view, but what will happens if at some point the contract is not respected and someone assume the $eventParameters exists? It's risky.

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 21, 2018

Contributor

Should be HookEvent instead Event. And you will be able to remove the test with instanceof

this is a BC, we can't do that.

Don't understand why it's a BC when there is an exception throw if it's not a HookEvent. Otherwise it's already a BC to add this condition.

you can make $eventParameters optional

this is a BC, we can't do that.

Can't be a BC. adding array $eventParameters = [] when it was array $eventParameters can't break anything.

Contributor

PierreRambaud commented Aug 21, 2018

Should be HookEvent instead Event. And you will be able to remove the test with instanceof

this is a BC, we can't do that.

Don't understand why it's a BC when there is an exception throw if it's not a HookEvent. Otherwise it's already a BC to add this condition.

you can make $eventParameters optional

this is a BC, we can't do that.

Can't be a BC. adding array $eventParameters = [] when it was array $eventParameters can't break anything.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

Don't understand why it's a BC when there is an exception throw if it's not a HookEvent.

yes, and it's part of the behavior: you can caught the exception while it's really hard to catch the error induced by the signature changes.

Can't be a BC. adding array $eventParameters = [] when it was array $eventParameters can't break anything.

If it changes nothing at all, why introduce a change?

Above all, I want to clarify what is done here: move what was previously in Services/HookDispatcher into Adapter namespace: there is no intent to refactor nor improve the current behavior here, as it's not tested at all. It was done only to comply with rules on src folder. You may suggest some improvements in Core implementation instead, don't you think?

Contributor

mickaelandrieu commented Aug 21, 2018

Don't understand why it's a BC when there is an exception throw if it's not a HookEvent.

yes, and it's part of the behavior: you can caught the exception while it's really hard to catch the error induced by the signature changes.

Can't be a BC. adding array $eventParameters = [] when it was array $eventParameters can't break anything.

If it changes nothing at all, why introduce a change?

Above all, I want to clarify what is done here: move what was previously in Services/HookDispatcher into Adapter namespace: there is no intent to refactor nor improve the current behavior here, as it's not tested at all. It was done only to comply with rules on src folder. You may suggest some improvements in Core implementation instead, don't you think?

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 22, 2018

Contributor

yes, and it's part of the behavior: you can caught the exception while it's really hard to catch the error induced by the signature changes.

My bad, forget the interface 🙄

If it changes nothing at all, why introduce a change?

It add a little value to call a function without adding and empty array if we don't have a parameters, don't forget we are not alone to use this project =)

Contributor

PierreRambaud commented Aug 22, 2018

yes, and it's part of the behavior: you can caught the exception while it's really hard to catch the error induced by the signature changes.

My bad, forget the interface 🙄

If it changes nothing at all, why introduce a change?

It add a little value to call a function without adding and empty array if we don't have a parameters, don't forget we are not alone to use this project =)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

If it changes nothing at all, why introduce a change?

ok, let's go for it

Contributor

mickaelandrieu commented Aug 22, 2018

If it changes nothing at all, why introduce a change?

ok, let's go for it

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

@PierreRambaud I've added the optional array 👍 and rebased and fixed coding styles

Contributor

mickaelandrieu commented Aug 22, 2018

@PierreRambaud I've added the optional array 👍 and rebased and fixed coding styles

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

This doesn't QA approval, in case of error it's on me :)

Thanks for review @PierreRambaud

Contributor

mickaelandrieu commented Aug 22, 2018

This doesn't QA approval, in case of error it's on me :)

Thanks for review @PierreRambaud

@mickaelandrieu mickaelandrieu merged commit b057cb5 into PrestaShop:develop Aug 22, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 26 files, 10 discussions left (mickaelandrieu, PierreRambaud, Quetzacoalt91, sarjon)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matks matks added the migration label Sep 18, 2018

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