civix#175 - Add support for shim scripts

[totten]

Overview

The civix code-generator provides a large set of boilerplate code (eg mymod.civix.php and mymod.php). Its purpose is to make civix code-conventions (e.g. *.mgd.php, xml/Menu/*.xml) work with civicrm-core APIs (e.g. hook_managed, hook_xmlMenu). In this way, it serves as a bridge, polyfill, or shim between a civix extension and core.

However, the boilerplate technique has various challenges. (See comments.)

This PR enables an alternative technique using "shim-files". A shim-file is a PHP file. As input, it receives some information about the extension (e.g. the extension-name and directory-path). It can analyze the extension. As output, it can register for hooks and call other APIs.

Compared to the boilerplate, shim-files aim to be gentler to work with at each stage of the lifecycle -- e.g.

• Prototyping - Instead of editing meta-PHP within civix.git, you can edit a regular PHP file in a regular extension.
• Distribution - Instead of re-generating a large boilerplate file from meta-PHP, and instead of manually updating random bits in a large boilerplate file... you can facilely copy small-grained files (without editing).
• Assimilation - If a shim-file is useful enough, then you can put the functionality in civicrm-core. This will automatically deactivate any obsolete shim-files bundled into extensions (without needing to republish the extensions).

See: totten/civix#175
Depends: #17831

Before

The civix templates generate two files, such as mymod.php (src:module.php.php) and mymod.civix.php (src:module.civix.php.php).

A typical example looks like this:

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

and

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

The actual files (mymod.php and mymod.civix.php) are much longer than the example - because they contain about a dozen shims.

These two files are managed differently:

• mymod.php is owned by the downstream developer, and they may add/remove/manage the hooks in this file. If any changes are required in this boilerplate, then the developer must make them manually. (See: https://github.com/totten/civix/blob/master/UPGRADE.md - "Special Tasks")
• mymod.civix.php is owned by civix and must be autogenerated. If any changes are required in this boilerplate, then the developer must regenerate. (See: https://github.com/totten/civix/blob/master/UPGRADE.md - "General Tasks")

After

When loading an extension, Civi will look for files named shims/*.shim.php. These are basic PHP files and do not require meta-PHP templating. Each file has an init function like:

return function(\CRM_Extension_ShimApi $shimApi) {
  // echo "This is " . $shimApi->longName . "!\n";
  \Civi::dispatcher()->addListener("hook_civicrm_xmlMenu", function($e) use ($shimApi) {
    foreach ((array) glob($shimApi->path . '/xml/Menu/*.xml') as $file) {
      $e->files[] = $file;
    }
  });
}

The shim-file has a few distinctive properties:

• It is loaded very early (at the same time as the extension's class-loader). This allows it to register for hooks.
• It receives some metadata about the extension ($shimApi). This allows it to examine the extension and register its artifacts.
• When/if civicrm-core provides the same functionality, then civicrm-core can disable/ignore/override the shim-file.
• It is a plain PHP file (not meta-PHP).
• It can be run multiple times and composed in different ways -- because it returns a callback and does not define any conflict-prone elements (class, interface, etc). (Analogy: The shim-file closure is similar to the Javascript closure used in a jQuery plugin.)

Depending on circumstance/taste, the shim-files may be written in a highly-coupled or loosely-coupled style:

• In highly-coupled style, you might have civix-convention-v1.shim.php with 10 different event listeners.
• In loosely-coupled style, you might have civix-menu-loader.shim.php, civix-mgd-loader.shim.php, and so on. Each file has only 1 or 2 listeners. These files are only created if they're needed.

Comments

Both approaches -- boilerplate/meta-PHP-template and shim-files -- can serve the role of bridge/polyfill/shim. You can write an extension using new code-conventions immediately (before they've been approved in core).

However, the boilerplate 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 adds hook_civicrm_foo, then it must add it to the boilerplate of all extensions... even for the many extensions which never use hook_civicrm_foo.
• If civix needs an update to its hook_civicrm_foo, then the author must regenerate mymod.civix.php.
• If civix's spin on hook_civicrm_xmlMenu becomes widespread, then the xmlMenu boilerplate is duplicated across many extensions.
• If you wish to canonize civix's xmlMenu scanner, then you might reproduce the scanner in civicrm-core. But this will cause a problem because all the previously published extensions still have their own copy of the scanner. Each *.xml file will be picked up twice, and there is no way to conditionally disable the old boilerplate.

With shim-files, one can still take advantage of new coding-conventions immediately. However, several of those pain-points are improved because shims are plain PHP files that can be debugged/copied/edited verbatim, and they can register their own hooks (without dynamic code generation). The code is no longer a "template", and it doesn't need to be interwoven between mymod.php and mymod.civix.php.

There is a presumption that all shims will be loaded. However, if the equivalent functionality has been migrated into civicrm-core, then civicrm-core may veto the shim-file.

Here's a slightly longer example of shim code: https://gist.github.com/totten/5fcd4835194981a6d667202d9e720cd5

Bike-shedding: The word "shim" mostly captures the intent, though other names could also describe this functionality - e.g. "mini module", "micro extension", "meta module"?

[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).

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@artfulrobot you look like you have gotten your head around this. I'm trying to resist the temptation not to just trust @totten on this & get some review but it makes my brain hurt

I don't think this needs very deep review in that it is adding code & won't break something & to the extent it does we'll make Tim fix it anyway and it only affects developers.

But I think it at least needs someone to +1 the approach & this seems to have emerged from a discussion you were in & are hopefully following

[artfulrobot]

@eileenmcnaughton I've been discussing this with @totten - me bringing crude ideas, Tim patiently applying and explaining genious!

Like you, it's a head scratcher, but I'm onboard with the theory now. It'll take me a while to look at this specific implementation (I have very limited time at the mo), and if y'all are ok with that there might be some value to me asking some more questions here.

Along the same lines, I've added needs-documentation because I think this sort of PR could be a problem if it went in without docs and then few people used it and we all forgot what it was supposed to do. I'd be happy to help write those, which would belong in the writing extensions chapter of the dev guide.

[eileenmcnaughton]

Thanks @artfulrobot - I have tried a few times but my head kept hurting

[artfulrobot]

@eileenmcnaughton I think it will become much simpler once we have some/more practical examples.

[eileenmcnaughton]

For sure - bluntly I think the goal is to use core code rather than copy & paste for repeated functions.

I suppose what I don't know is how tricky that is to keep version compatibility on

[totten]

Thanks and agree with the needs-documentation and the value of a more complete example.

Made some updates to the PR description.

Rebased - since the related PR is now merged, this one now looks smaller than it did (1 commit vs 3 commits).

A few notes-to-self/review:

• The draft is a bit mum about how to bundle shim-files into civicrm-core. I think it should make more commitment on that and demonstrate it concretely.
• While I still agree with the main thrust that this mechanism allows code to transition more naturally between prototype<=>civix<=>core, there is a hole in this draft. Consider the pre-existing convention of *.mgd.php... even if we follow the shim-file pattern, there are still a bunch of pre-existing codebases with the boilerplate. This can still lead to double-scans. I think the resolution is to discriminate between extensions - older ones (with current boilerplate) don't use any shims (even if they're defined in core), but newer extensions do use shims. (Detail TBD)
• It should probably have a test. Maybe an E2E test would be workable.
• When scanning for extensions+shim-files, it may be tempting to build on CRM_Core_Module::getAll() instead of CRM_Extension_Manager::getStatuses(). This would allow shim-files to be applied to CMS packages (modules/plugins). But it would require other updates, and I'm not sure all CMS's even provide base-paths for their modules, and it might have bootstrap complexities... so perhaps not worth the effort.
• There is a subtle discrepancy between function mymod_civicrm_foo() and Civi::dispatcher()->addListener('hook_civicrm_foo')... eg Civi::reset() will teardown the latter but preserve the former. It doesn't matter for normal page-loads, but it bears consideration for unit-testing. (To wit: will we have quirks where the listeners show up in the first unit-test but disappear in the second unit-test?)

[eileenmcnaughton]

test this please

[eileenmcnaughton]

Closing for now - @totten to re-open once above addressed