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

civix#175 - Add support for mixins. Switch core extensions to mixin/setting-php #22198

Merged
merged 8 commits into from
Dec 1, 2021

Conversation

totten
Copy link
Member

@totten totten commented Dec 1, 2021

(This is an update of #17832 and #19865. Broadly, it addresses issues identified in those PRs and goes further - adding automated tests, allowing civicrm-core to define mixins, and converting several core extensions to use the first mixin setting-php.)

Overview

civix has traditionally use code-templates to generate a large amount of boilerplate. Some of this boilerplate is entirely redundant, and propagating updates for it is difficult. With many extensions, the cumulative weight can become non-trivial.

This PR introduces an alternative, "mixins". A mixin is a small file that enhances an extension. Loosely speaking, a "Civi mixin" enhances a "Civi extension" in the same way that a "PHP trait" enhances a "PHP class" - by mixing-in various methods/hooks/etc.

Mixins are simple, static PHP files that can be edited and debugged. Mixins are named, versioned, and deduplicated. They are not code-templates. They do not use scans/file-loads/hooks unless an extension actually needs the functionality.

Mixins still allow strong backward/forward interoperability. A developer (or dev-tool like civix) may copy a mixin to an extension and deploy it on an older version of civicrm-core (even without this PR). However, core support has several benefits: improved versioning/upgrading, improved caching, and so on. Additionally, it allows core extensions to take advantage of mixins.

See also: 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;
  }
}

After

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

<extension key="org.example.mymod" type="module">
  <file>mymod</file>
  <mixins>
    <mixin>menu-xml@1.0.0</mixin>
  </mixins>
</extension>

The name menu-xml@1.0.0 is used to load an eponymous PHP file. This a simple/static PHP file, and it looks like this (edited for brevity):

return function(\CRM_Extension_MixInfo $mixInfo, \CRM_Extension_BootCache $bootCache) {
  Civi::dispatcher()->addListener('hook_civicrm_xmlMenu', function ($e) use ($mixInfo) {
    $files = (array) glob($mixInfo->getPath('xml/Menu/*.xml'));
    foreach ($files as $file) {
      $e->files[] = $file;
    }
  });
};

One copy of this menu-xml@1.0.0 file is sufficient for use by multiple extensions.

FAQ

Q: What mixins are defined in this PR?

This PR only defines one mixin (setting-php) which autoloads $EXT/setting/*.setting.php files. It also updates all core extensions to (a) remove the old setting boilerplate and (b) use the setting-php mixin if necessary.

The follow-up PR #22199 converts several more functions from boilerplate-notation to mixin-notation.

Q: What test coverage is there?

There are generally two forms of coverage:

  • By adopting the mixins for core extensions, we gain exposure through any existing test-coverage for those extensions. You can do some r-run with the core extension (enabling+disabling them; viewing admin screens) to see that the settings still work.
  • The folders like mixin/setting-php@1/example (et al) define example files. The script ./tools/mixin/bin/test-all will generate a test extension, copy in the examples, and assert that the examples work.

Q: Why must an extension opt-in to using <mixin>menu-xml@1.0.0</mixin>? Why not automatically scan $EXT/xml/Menu/* in all extensions?

A: Automatically scanning $EXT/xml/Menu/* in all extensions would interfere with pre-existing extensions - because they already have their own scanners and loaders. Consider a site that has an old copy of org.example.mymod installed. It defines function mymod_civicrm_xmlMenu() to scan for mymod/xml/Menu/*. If civicrm-core automatically scanned $EXT/xml/Menu/* on all extensions, then mymod/xml/Menu/* would load twice.

Depending on the happenstance of the specific hook/scanner, this may produce duplicate registrations, integrity errors, etc. Theoretically, one might be able to negotiate this conflict for specific hooks+files. However, using an opt-in is a simple technique that categorically works for many different hooks/scanners/file-conventions, and it lines us up well if we need to make future changes.

Q: Where are mixin files loaded from?

Generally:

BASEDIR/mixin/NAME@VERSION.mixin.php   (*dot-mixin-php*)
BASEDIR/mixin/NAME@VERSION/mixin.php   (*sub-dir*)

For example, on a D7/BD site, one might have:

BASEDIR                                   MIXIN FILE
sites/default/files/civicrm/ext/foobar/   mixin/menu-xml@1.1.0.mixin.php
sites/default/files/civicrm/ext/whizbang/ mixin/menu-xml@1.0.3.mixin.php
sites/all/modules/civicrm/                mixin/menu-xml@1/mixin.php (with header `@mixinVersion 1.3.9`)
sites/all/modules/civicrm/                mixin/menu-xml@2/mixin.php (with header `@mixinVersion 2.0.1`)

This example includes four versions (1.1.0, 1.0.3, 1.3.9, and 2.0.1). Older versions (1.1.0, 1.0.3) are effectively ignored. Depending on the <mixin> declaration, it will load the best compatible version - either 1.3.9 or 2.0.1. (Under SemVer, MAJOR increments are not mutually compatible. But MINOR and PATCH versions have implied forward-compatibility.)

Q: How do I backport a mixin for use on an older version of civicrm-core?

A: I don't actually recommend that most developers do this. An upcoming version of civix will do it automatically/as-needed.

But the mechanism is fairly simple: Copy the mixin file, and ensure that the full version is included.

CIVI="sites/all/modules/civicrm"
EXT="sites/default/files/civicrm/ext/org.example.mymod"
cp "$CIVI/mixin/xml-menu@1/mixin.php" "$EXT/mixin/xml-menu@1.0.0.mixin.php"

Additionally, if the target version of civicrm-core does not have mixin support, then you need to copy/configure mixin/polyfill.php.

…boot-cache.

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:

```php
// 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

```php
// 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:

```xml
<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:

```php
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.

Technical Details
-----------------

Mixins may define internal classes/interfaces/functions. However, each major-version
must have a distinct prefix (e.g. `\V2\Mymixin\FooInterface`). Minor-versions may be
provide incremental revisions over the same symbol (but it's imperative for newer
increments to provide the backward-compatibility).

MixinScanner - Make it easier to instantiate and pay with instances

Ex: cv ev '$o=new CRM_Extension_MixinScanner(); var_export($o->createLoader());'

MixinScanner - Enable scanning of `[civicrm.root]/mixin`
@civibot
Copy link

civibot bot commented Dec 1, 2021

(Standard links)

@totten
Copy link
Member Author

totten commented Dec 1, 2021

jenkins, test this please

1 similar comment
@totten
Copy link
Member Author

totten commented Dec 1, 2021

jenkins, test this please

*/
function shimmy_civicrm_entityTypes(&$entityTypes) {
_shimmy_civix_civicrm_entityTypes($entityTypes);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit ironic that the shimmy extension has a civix file implementing the boilerplate that the extension is designed to get rid of :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh :) Yeah, the process I followed was basically:

  1. Make an extension (civix generate:module shimmy) with lots of boilerplate functions.
  2. Read through the boilerplate functions.
  3. One-by-one, delete each function and write a mixin. (Most of these were cookie-cutter changes.)

The leftover file is like a TODO list ("Things to reorganize!"). A lot of the remaining boilerplate can be nixed, but they're not quite so cookie-cutter.

  • Install/enable/upgrade stuff should use <upgrader>+<classloader> instead of <mixin>.
  • The include_path and entityTypes stuff should be amenable to moving to a mixin, assuming we want to keep those code-patterns.
  • The insert_navigation_menu() stuff is wonky. We can probably delete the fixNavigationMenu straight-up.
  • That leaves CRM_*_ExtensionUtil and _*_civix_mixin_polyfill as things to preserve for the long term.

@colemanw
Copy link
Member

colemanw commented Dec 1, 2021

Based on many discussions and reading the code, I think this is ready to merge. In particular I note that this has good test coverage, the new tests are passing, and this also refactors other extensions such as financialacls which have their own unit tests which would fail if this didn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants