CRM-21341 Drupal 8 Hook Support

[kerasai]

Overview

This PR adds support for calling CiviCRM hooks provided by Drupal modules in a Drupal 8 installation.

Before

Code for CiviCRM hooks provided by Drupal 8 modules never gets called.

After

Code for CiviCRM hooks provided by Drupal 8 modules now gets called after adding the appropriate code to make CiviCRM aware of the Drupal 8 modules.

Technical Details

I've refactored \CRM_Utils_Hook_DrupalBase to include a ::getDrupalModules method that calls the existing Drupal code for obtaining the module list. Then I built out \CRM_Utils_Hook_Drupal8 class to override the method with the Drupal 8 equivalent code for obtaining the list of modules.

---

• CRM-21341: Drupal 8 Hook Support

[civicrm-builder]

Can one of the admins verify this patch?

[jackrabbithanna]

ooh yeah, I'll test this out this weekend, awesome :)

[kerasai]

@jackrabbithanna I was also looking into supporting event subscribers, which are now utilized in Drupal as of D8. I'm not seeing anything in any of the CMSs' integration that looks to provide this. Is there anything you may be able to point me towards?

[jackrabbithanna]

@totten if i"m not mistaken you were planning or developing use of Symfony's events and event subscribers for CiviCRM, unrelated to D8, correct? Perhaps you can help answer @kerasai ?

[seamuslee001]

Jenkins ok to test

[jackrabbithanna]

Created JIRA ticket for this PR
https://issues.civicrm.org/jira/browse/CRM-21341

[jackrabbithanna]

I've confirmed this PR works, I created a small test module that implements hook_civicrm_post() and write a log entry to the dlog....
I'll post it here in case anyone wants to try it out.
civicrm_hook_test.tar.gz

[jackrabbithanna]

@kerasai We may want to keep this PR to adding hook support, and perhaps create another JIRA issue for event subscribers. As a standard practice it is recommended to create JIRA issues at https://issues.civicrm.org ...

I've been adding the label 'drupal8' to issues that are for Drupal 8 development.

If you don't have access to create issues in JIRA or need help, ping me on https://chat.civicrm.org , infrastructure channel, and I can help or point you to somebody that can help....

Then we can have a separate discussion about the event subscribers on the JIRA ticket, and this PR can get merged and others get the functionality while discussions take place.

@colemanw or @totten , I've reviewed the code and verified functionality. Good to merge in my opinion.

[kerasai]

@jackrabbithanna, I agree--the work/discussion for the event subscribers should be split off to another PR/issue in order to get this work for the traditional "hooks" in.

I don't have access to the JIRA site, have been waiting on approval for a few weeks now. I'd be glad to create an issue there, and document a plan for implementation.

[totten]

A couple things...

• @kerasai - I think I found your account on civicrm.org -- it should start working for JIRA logins now. 🤞
• For background on the Symfony Event support in CiviCRM, these might be interesting:
  • Dev Docs: Hooks with Symfony (newest and most concrete doc)
  • PR #9949: Unify Symfony Events and hooks
  • Forum: Proposal: Consolidate event and declaration architectures (oldest and most abstract doc)
• There's a small voice in my head saying, "Drupal 8 code can just use Civi::dispatcher(). You don't need hook-integration." Alas, that voice is not quite right -- because there are three boot-critical cases that only dispatch through the hook system (hook_civicrm_config, hook_civicrm_container, hook_civicrm_entityTypes). And in pragmatic terms, forward-porting the hook-integration to D8 is the simplest way to handle these.
• There's another small voice in my head saying, "CRM/Utils/Hook/Drupal*.php is breaking encapsulation... it should be call module_invoke_all() instead of module_list()." This voice might be wrong with respect to oddballs like hook_civicrm_validateForm and hook_civicrm_dashboard. (Those functions return arrays, and the results need to be merged appropriately.)
• Overall, the code-style seems acceptable+consistent 👍 , and I'll take @jackrabbithanna's word that the patch works as advertised. Let's wait a day in case those last two points conjure some important reaction -- but, otherwise, I think it's OK to merge this patch.

[seamuslee001]

@totten agree with you on all parts, I think the reason between module_invoke_all and module_list is that we are trying to get a list of modules and combine that with a list of extensions so that when we invoke the hook we invoke for all at once perhaps?

[dsnopek]

I think this should be changed to:

if (class_exists('\Drupal') && \Drupal::hasContainer()) {

Otherwise, I'm seeing this line crash when running 'cv' on a Drupal 8 site because this gets called before 'cv' has a chance to bootstrap Drupal

[dsnopek]

I pushed a new commit to fix the issue I commented about :-)

[jackrabbithanna]

So I've let this stew long enough, lets get it in, thanks @dsnopek for the update (and reminder)
With a review, and a test, and @tottens blessing
Merging #11171

[tottens]

@jackrabbithanna Sorry, I'm not involved in this project, so I'm unable to comment on or give my blessing to @dsnopek 's change. Perhaps you meant someone else.

[jackrabbithanna]

ok sorry, 14 days ago you said it was ok, the change is an improvement on what already worked

[kerasai]

@jackrabbithanna different users, nearly identical usernames.

A new type of off by one error ;)

[jackrabbithanna]

Oh I see now...there are multiple totten(s) :) ?

[tottens]

@jackrabbithanna I need to find out who is this doppelganger of mine! :)