use of Symfony EventDispatcher instead of CRM_Utils_Hook

[nganivet]

This introduces YAC (Yet Another Construct) that developers will have to learn with no perceived benefit over the current hook system. Most of our developers come from the CMS world and are used to the hook construct. Despite the explanation in the README.md, I see no good reason to introduce another construct.

If the concept of priority listener and vetoing is really useful for CiviCRM, then let's introduce this in the hook system! This could be done quite easily with the hook cache in 4.7 (https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Hook.php#L209).

See https://issues.civicrm.org/jira/browse/CRM-19813 for implementing 'core hooks' based on the above and further enabling the LExIM strategy.

[totten]

1. See also: https://forum.civicrm.org/index.php?topic=36086.0

2. Symfony EventDispatcher was introduced to civicrm-core several versions ago (4.4?). I suppose the difference is that we're publicly documenting some events here.

3. Symfony EventDispatcher is used by a ton of PHP projects -- including Drupal 8. It's the closest thing there is to a standard in PHP events.

4. I don't have any idea how you would introduce prioritization/vetoing on top of CRM_Utils_Hook without having the same problem that you just complained about -- introducing something that feels new/weird/redundant to a CMS dev. As an intellectual curiousity, that would be something interesting to see. However... if we have to do something different from status quo, better to align with the common practice among PHP apps.

5. I think most of us have incorrect intuitions because we learned about Civi hooking through some particular binding (e.g. Drupal hooks or Joomla events or WP actions). For example: if you first learned Drupal hooks, then you'd assume there's a choice of functionality like module_invoke_all() or module_invoke() or module_implements(). But there's not - you can't do module_implements() in Joomla without patching core or writing a parallel system, and the concept is totally bogus in WP. (On the flip-side, WP developers assume you can rejigger events in ways that you can't -- because that wouldn't make sense in Drupal. And D7 developers have suffered some stockholm syndrome in embracing module-weights.) I've been bitten by this more than once when trying to design good hooks. The end result is that you can only work with the least common denominator across all event systems.

[nganivet]

Tim - then let's obsolete the hook system in favor of events. But keeping both is only confusing and adds significantly to the CiviCRM learning curve as well as things that need to be tested/can break in each release. We must stop having 5 different ways of doing anything in CiviCRM because of NIH or 'I know better' syndrome. CiviCRM ends-up being the unmaintainable patchwork of half-baked ideas that it currently is.

[nganivet]

Tim - see https://forum.civicrm.org/index.php?topic=36086.msg157211#msg157211. Suggestion a) which is your 4) above is trivial to implement. a) is incremental, b) is revolutionary, but IMO either are preferable to the current YAC situation.

[tttp]

Not trying to hijack this discussion but trying to compare hook vs. dispatcher for our needs:

• being able to alter the tracked url (eg NOT tracking urls from blacklisted domains, add utm_ GA params on some others)
• being able to alter tracking urls (ie. instead of having domain.org/sites/all/modules/civicrm/external/url.php?q putting something shorter)
• being able to alter the open url (adding a bloom filter on (contact_id, ip) to be able identify if a message is forwarded)

@nganivet on 4.6 we did try to do it via hooks, it felt rather clunky (don't recall the details, we needed to alter the message composition too)

@totten would EventDispatcher make it easier to cover these cases?

[totten]

1. Open Tracking

The default open-tracking is handled through a service (civi_flexmailer_open_tracker aka Civi\FlexMailer\Listener\OpenTracker which listens for ComposeBatchEvent. To track opens differently, I think your extension could add a new class:

namespace Civi\UberTrack;
class UberOpenTracker extends BaseListener {
  public function onCompose(ComposeBatchEvent $e) {
    if (!$this->isActive() || !$e->getMailing()->open_tracking) {
      return;
    }

    Civi::service('civi_flexmailer_open_tracker')->setActive(FALSE); // Opt-out of default

    foreach ($e->getTasks() as $task) {
       // Add code using $task->getMailParam('html') and $task->setMailParam('html').
     }
  }
}

...

use Civi\FlexMailer\FlexMailer as FM;
$container->setDefinition('uber_open_tracker', 
  new Definition('Civi\UberTrack\UberOpenTracker'));
$container->findDefinition('dispatcher')->addMethodCall('addListenerService', array(
  FM::EVENT_COMPOSE,
  array('uber_open_tracker', 'onCompose'),
  FM::WEIGHT_ALTER+10)
);

Notably, this only works as long as the weight of UberOpenTracker places it before OpenTracker. If the order is transposed, then the opt-out (setActive(FALSE)) would come too late. (Weights are numerical, and it may not be obvious if you chose a good number. You can twiddle the weights and run cv debug:event-dispatcher /flexmail/ to see if it works out.)

I'm a bit on the fence about the best notation for opting out of civi_flexmailer_open_tracker. The setActive(FALSE) works and seems simple. However, it might be elegant and generally useful if you could actually swap-out the implementation of civi_flexmailer_open_tracker... but I don't think that'll work reliably right now.

I mention the swap-out issue because you also asked about click-tracking ... though it may take a moment to explain.

2. Click Tracking

Click-tracking is also handled by a pair of services:

• civi_flexmailer_html_click_tracker (HtmlClickTracker.php)
• civi_flexmailer_text_click_tracker (TextClickTracker.php)

Right now, these services have a special place -- click-tracking is quite finicky about when we apply the transformation. URLs can have dynamic tokens which are tied into the templating language; to get backward-compatible semantics in the default scenarios, I had to apply click-tracking at a specific place in DefaultComposer.

To actually change the click-tracker, we probably need some kind of improvement. Options:

• If we had a more general way to swap-out a particular service, then you could replace civi_flexmailer_html_click_tracker with something different. It just needs to implement the function filterContent().
• We could try to update FlexMailer so that civi_flexmailer_html_click_tracker becomes an event-listener. Then the mechanics for overriding would be the same.

3. Others

You asked about open-tracking and click-tracking as examples. I think open-tracking is more representative because several other features follow the same pattern: basic headers, bounce-tracking, template-evaluation, and attachments are all handled by similar services. (They monitor EVENT_COMPOSE and can be toggled with setActive().)

[tttp]

Did we move forward on hooks vs. event dispatcher? What about

• changing the hooks so it always dispatch an event now
• document it first (I'm volunteering to write a blog post) and put it first in the wiki
• in v+1, start adding a warning every time we find a civicrm_example_hook in the log
• in v+2, display the message if UI+admin
• in v+3, do not call the civicrm_example_hook events probably changing civix somewhere near v+1 so generate event dispatchers?

[nganivet]

Xavier - Exciting: Tim has recently committed this to core! The Symphony event dispatcher is now the main 'event bus' in CiviCRM and triggering the legacy hooks. The events triggered by core are a superset of the hooks, and extensions can define their own events. You can get to Tim for more info.

[totten]

For a bit of follow-up, https://github.com/civicrm/civicrm-core/pull/9949 unified Symfony Events and hooks. Now, events are canonically dispatched through Symfony. If the event is named hook_*, then Symfony will pass it along to the CMS's event system. Effect: we have one logical set of events with multiple syntaxes/bindings. The Symfony binding is the most nuanced/flexible.

This unification leaves a question open for FlexMailer -- perhaps we should rename the FlexMailer events to follow the hook_* convention and extend GenericHookEvent...

[nganivet]

Tim - It depends on whether you want extensions to migrate to Symfony events over time, or leave Symfony just for core usage. If the former, then we have to create all new hooks as Symfony events, write documentation, make an announcement, do some training, migrate all examples and sample extensions to events, and obsolete old hooks over time. If the later (or still undecided), then I would advise that we rename the FlexMailer events and document these in the develop[er guide as hook_*.

[nganivet]

Tim - note my current thinking is that, as 'core extension' are created, extension developers will need to be able to override and/or prioritize their extensions over the ones from core. So for example:

• if I don't like the way the Financial ACLs are implemented by core I can override these through another extension. It might be as simple as disabling the core extension are enabling mine.
• if I need to determine the congressional district for a contact and save it in it's record I might want to use the hook_post, but only be triggered after the 'core Geocode extension' has been run. It's easy to imagine other use cases for which the opposite would be true.

I do not know if this is easier implemented from Symfony, or if we can 'pass-through' the Symfony prioritization to the hook_* extension system. This might help determine the answer to my previous comment.

[totten]

Yeah, I probably should rename them and document as hook_*. I guess it's on the critical-path to Mosaico 2.0-stable.

Regarding the ordering of events, the key things to know are:

• Symfony Events are dispatched with a numerical priority (fired in descending order).
• To see the effective ordering, run cv debug:event-dispatcher /hook_civicrm_post/ (or similar).
• All listeners registered with Civi::dispatcher()->addListener() have configurable priority. The default priority is 0.
• All listeners registered with Drupal/Joomla/WordPress/Extension-style have a fixed priority, -100, aka DEFAULT_HOOK_PRIORITY. These appear in the debug console as one step, delegateToUF().
• If the DEFAULT_HOOK_PRIORITY is inappropriate, then switch to Civi::dispatcher()->addListener() notation.

[totten]

Relevant PR and critique: https://github.com/civicrm/org.civicrm.flexmailer/pull/7

[totten]

Doing #8 instead.