Skip to content
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

(NFC) CiviEventDispatcher - Update type declaration. Add test demonstion #26276

Merged
merged 2 commits into from
May 24, 2023

Conversation

totten
Copy link
Member

@totten totten commented May 19, 2023

Overview

This is a revision of @dontub's #26212. (cc @demeritcowboy)

Before

dispatcher($eventName, $event) hints that the $event is type Event. This does not resolve to a valid class. (\Civi\Core\Event does not exist.)

After

dispatcher($eventName, $event) hints that the $event is type object.

Technical Details

  • It already works, as you can see in the unit-test. This is because we've inherited behavior from Symfony 4.4+ (which supports PSR-14).
  • PSR-14 recommends modeling events as any kind of object.
  • There's currently no need for Civi to specifically have a PSR-14 adapter or integration. But this doesn't hurt us either.
  • https://en.wikipedia.org/wiki/Robustness_principle

@civibot
Copy link

civibot bot commented May 19, 2023

(Standard links)

@civibot civibot bot added the master label May 19, 2023
@demeritcowboy
Copy link
Contributor

The main reason for encouraging GenericHookEvent (or at least something civi controls) is so that extension authors can be insulated from whatever symfony/drupal/php does next, and given the above links there's a wide range of things they could do. By using whatever object Civi is using, they know their code will be compatible across supported versions. Of course civi could change what it uses too, but it would least make an attempt to stay compatible.

It can dochint object, but then it's up to authors to keep up with whatever they need to do to be cross-compatible. So I'm still wondering if it should more strongly suggest using GenericHookEvent, although I do understand the comments in the other PR that that class might include stuff that isn't wanted.

For what it's worth object (currently) works on drupal 10/symfony 6 too.

@totten
Copy link
Member Author

totten commented May 20, 2023

@demeritcowboy - Yeah, the cross-compatibility is an important consideration. I took another stab at the description.

@totten totten force-pushed the master-dispatch-sig branch from 831d613 to 2bbd4e8 Compare May 20, 2023 04:05
@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label May 20, 2023
@demeritcowboy
Copy link
Contributor

Have put merge-ready in case @dontub has any comments.

And can think about this after but a followup might be to add a deprecation warning to dispatch() if someone is passing in a Symfony\Component\EventDispatcher\Event (as opposed to \Symfony\Contracts\EventDispatcher\Event), because that isn't forward-compatible with symfony 6. A quick look at universe I see at least one.

@totten
Copy link
Member Author

totten commented May 24, 2023

OK, so no more comments. And it's NFC. Merging.

(@demeritcowboy) ...a followup might be to add a deprecation warning to dispatch() if someone is passing in a Symfony\Component\EventDispatcher\Event...

Oof, yeah, I found ~12 extensions in my copy of universe. Probably a good idea.

@totten totten merged commit 98ab709 into civicrm:master May 24, 2023
@totten totten deleted the master-dispatch-sig branch May 24, 2023 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants