(WIP) civix#175 - Add support for mixins

[totten]

(This is an update of #17832 which addresses several of the issues from my last comment on that PR. However, there's still an issue which I haven't fully got my head around - which will discuss in a comment. But for the moment, it'd be nice to see how the tests run.)

Overview

(NOTE: For this description, I reference the term "API" in the general sense of a programmatic interface -- such as a hook or file-naming convention. It is not specifically about CRUD/DB APIs.)

The civix code-generator provides support for additional coding-conventions -- ones which are more amenable to code-generation. For example, it autoloads files from xml/Menu/*.xml and **/*.mgd.php. The technique for implementing this traditionally relies on generating a lot of boilerplate.

This patch introduces a new construct ("mixin") which allows boilerplate to be maintained more easily. A mixin inspects an extension programmatically, registering new hooks as needed. A mixin may start out as a file in civix (or even as a bespoke file in some module) - and then be migrated into civicrm-core. Each mixin has a name and version, which means that (at runtime) it will only load the mixin once (ie the best-available version).

See: totten/civix#175

Before

The civix templates generate a few files, such as mymod.php and mymod.civix.php. A typical example looks like this:

// mymod.php - Implement hook_civicrm_xmlMenu
require_once 'mymod.civix.php';
function mymod_civicrm_xmlMenu(&$all, $the, $params) {
  _mymod_civix_civicrm_xmlMenu($all, $the, $params);
}

and

// mymod.civix.php - Implement hook_civicrm_xmlMenu
function _mymod_civix_civicrm_xmlMenu(&$all, $the, $params) {
  foreach (_mosaico_civix_glob(__DIR__ . '/xml/Menu/*.xml') as $file) {
$files[] = $file;
  }
}

These two files are managed differently: mymod.php is owned by the developer, and they may add/remove/manage the hooks in this file. mymod.civix.php is owned by civix and must be autogenerated.

This structure allows civix (and any civix-based extension) to take advantage of new coding-convention immediately. However, it comes with a few pain-points:

• If you want to write a patch for _mymod_civix_civicrm_xmlMenu, the dev-test-loop requires several steps.
• If civix needs to add a new hook_civicrm_foo, then the author must manually create the stub function in mymod.php. civix has documentation (UPGRADE.md) which keeps a long list of stubs that must be manually added.
• If civix has an update for _mymod_civix_civicrm_xmlMenu, then the author must regenerate mymod.civix.php.
• If mymod_civix_xmlMenu needs a change, then the author must apply it manually.
• If civix's spin on hook_civicrm_xmlMenu becomes widespread, then the xmlMenu boilerplate is duplicated across many extensions.

After

An extension may enable a mixin in info.xml, eg:

<mixins>
  <mixin>civix-register-files@2.0</mixin>
</mixins>

Civi will look for a file mixin/civicrm-register-files@2.0.0.mixin.php (either in the extension or core). The file follows this pattern:

return function(\CRM_Extension_MixInfo $mixInfo, \CRM_Extension_BootCache $bootCache) {
  // echo "This is " . $mixInfo->longName . "!\n";
  \Civi::dispatcher()->addListener("hook_civicrm_xmlMenu", function($e) use ($mixInfo) {
...
  });
}

The mixin file is a plain PHP file that can be debugged/copied/edited verbatim, and it can register for hooks on its own. The code is no longer a "template", and it doesn't need to be interwoven between mymod.php and mymod.civix.php.

It is expected that a system may have multiple copies of a mixin. It will choose the newest compatible copy. Hypothetically, if there were a security update or internal API change, core might ship a newer version to supplant the old copy in any extensions.

Comments

https://github.com/totten/shimmy/commits/master is an extension that a sample extension with an example mixin and significantly reduced boilerplate (shimmy.civix.php, shimmy.php).

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

There's something that I have trouble wrapping my head around. I don't think it's unique to the stuff in this PR, but mixin happens to lean into the affected case more heavily.

Suppose you have an extension which does some dynamic-ish registration, eg

// FILE: myext.php

Civi::dispatcher()->addListener('hook_civicrm_alterMenu', function($e) {
  $e->items['foo'] = [...];
});

Now, suppose you uninstall this extension. It seems this would be the sequence of events:

1. Bootstrap Civi
   a. Load myext.php, which adds listener to Civi::dispatcher() (which is stored statically)
2. Run the uninstall routine
   a. Change extension status to disabled
   b. Run rebuildMenuAndCaches, which in turn repopulates the menu and fires hooks

The thing is that we don't do Civi::reset(), so the singleton-ish data in Civi::dispatcher() and Civi::container() is still there. Which means that step 2.b will fire the listener from 1.a -- even though the extension which registered it is inactive.

(I suspect there may be some parallel issues around the management of singleton-ish data-structures during extension-lifecycle. Possibly (a) when manipulating the container definitions and/or (b) when the lifecycle action is install/enable.)

(Why isn't this a concern for traditional hook-listeners? Because they're filtered on the list of active extensions, which is modified in step 2.a.)

My current draft mixin in shimmy/mixin/civix-register-files is basically doing the same thing, but it works-around this by adding a few isActive() guards on its new hooks. I just have a nagging suspicion that there's some other hole in the lifecycle -- and that we'll ultimately need one of these:

• When performing install/etc, the whole of Civi::$statics (incl dispatcher+container) should be torn and recreated. Or...
• The extension interface should have more lifecycle events -- in addition to install/enable/disable/uninstall, there should be a lower-level load/unload that applies to event-listeners/static-state within the current page-view. Or...
• When performing install/etc, Civi should only clear out caches within the current page-view. All re-hydration should be deferred to the next page-view. (Though this might not be workable for unit-testing...)

[eileenmcnaughton]

@totten is this still WIP?

[colemanw]

@totten needs rebase

[demeritcowboy]

Going thu some old inactive PRs and am going to close this but feel free to reopen. Patch is stale.