(NFC) mixin/**.php - Add @since tags

[totten]

Overview

Adds extra metadata to the mixin files.

This will enable better code-generation -- ie civix would be able to skip unnecessary backports. It is an off-shoot from discussion in veda-consulting-company/uk.co.vedaconsulting.mosaico#520

NOTE: This PR builds on #23422, so you'll see 1-2 extra lines in the diff. It does not fundamentally depend on 23422, but they touch on adjacent annotations, and they would conflict if merged in parallel. One has to go before the other. This one can go second.

Before

Hard to tell when a mixin was introduced (unless you go poking through git history).

After

Each mixin reports when it was introduced (@since).

Technical Details

You can inspect the parsed metadata and see the 'since' value:

$ ./tools/mixin/bin/mixer list

NAME                 VERSION  SINCE    DESCRIPTION
----                 -------  -----    -----------
afform-entity-php    1.0.0    5.50     Auto-register "afformEntities/*.php" files.
ang-php              1.0.0    5.45     Auto-register "ang/*.ang.php" files.
case-xml             1.0.0    5.45     Auto-register "xml/case/*.xml" files.
menu-xml             1.0.0    5.45     Auto-register "xml/Menu/*.xml" files.
mgd-php              1.1.0    5.50     Auto-register "**.mgd.php" files.
setting-php          1.0.0    5.45     Auto-register "settings/*.setting.php" files.
theme-php            1.0.0    5.45     Auto-register "*.theme.php" files.

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

[colemanw]

@totten one question about this: the mgd-php mixin has actually been around since 5.45 but there's no annotation anywhere to say so, which means that civix will assume it isn't available at all < 5.50, correct? (not that civix currently uses the @since value for anything, but if it did...)

[colemanw]

@totten thinking about this some more, I'm not sure how civix is going to make sense of the @since annotations if they keep changing with every minor version bump. Maybe you have a different plan already worked out, but my understanding of how these annotations generally get used is to find out when a service became available at all, not when it received the last minor version bump. So I think the annotations out to look like this:

• ang-php v1.0.0: @since 5.45
• mgd-php v1.0.0: @since 5.45
• mgd-php v1.1.0: @since 5.45
• mgd-php v2.0.0: @since 6.97

And in that example, the 1.0.0 version of mgd-php gets replaced in our release tarball by 1.1.0, but 1.1.0 and 2.0.0 both live in the tarball side-by-side.

[totten]

@colemanw Good point. I was wondering about that too. We should probably break that down as a couple distinct things:

• Tracking accurate metadata about the mixin (itself): Historically, we didn't describe these files completely. ([email protected]:/mixin/** does not have any annotations like @since 5.45.) We should maybe add @since tags to [email protected]:/mixin/** (or the nearest live branch) to make the metadata more accurate.
• Choosing a target-version for backports: When you generate some code, it might write two artifacts in the extension (eg the main artifact foobar.mgd.php and a backport file mixin/mgd-php.X.Y.Z.mixin.php). But which backport file should we consider writing?
  • The effective outcome is dictated by civix:composer.json -- this identifies the file that civix will copy/backport.
  • In this particular case, the on-disk format for foo.mgd.php has not changed at all, and both versions of mgd-php will work with any version of civicrm-core. We have full backward/forward interoperability on both axes. So the decision doesn't matter much. We could just flip a coin.
  • Hypothetically... there could be a future update ([email protected]) which adds new features. Maybe a future civix wants to use those new features. For[email protected], we would both (a) update the code-generator that produces foobar.mgd.php and (b) update the backport file (listed in civix:composer.json).

For the current situation... Perhaps this would be a reasonable approach:

1. Update [email protected] to fix/fill-in the metadata (@since 5.45).
2. Update civix so that it still uses the old version of [email protected] (but with the new/filled-in metadata).

[colemanw]

@totten was that a response to my 1st comment or my 2nd?

[totten]

(@colemanw) was that a response to my 1st comment or my 2nd?

Crossed wires. It's a reply to the first comment, which came through about the same time you posted the second comment.

(@colemanw)... to find out when a service became available at all, not when it received the last minor version bump. So I think the annotations out to look like this...

Right, important distinction. For making the optimization in mosaico#520, I think it's more useful to note when this particular revision was merged into core.

• [email protected]:mixin/ang-php/mixin.php: @mixinVersion 1.0.0 @since 5.45
• [email protected]:mixin/mgd-php/mixin.php: @mixinVersion 1.0.0 @since 5.45
• [email protected]:mixin/mgd-php/mixin.php: @mixinVersion 1.1.0 @since 5.50
• [email protected]:mixin/mgd-php/mixin.php: @mixinVersion 1.2.0 @since 5.55
• [email protected]:mixin/mgd-php/mixin.php: @mixinVersion 2.0.0 @since 6.97

But if the semantic of @since is different... could we just change the annotation name?

[colemanw]

For making the optimization in mosaico#520, I think it's more useful to note when this particular revision was merged into core.

This makes me think you have a plan in your head that I'm not privy to. Because I can't imagine why civix would need to know that mgd-php received a minor update in 5.50. Couldn't it just check the latest master branch of civicrm-core, look up the @since version of the mgd-php to find out that 1.x is available since 5.45? What else does it need to know?

[totten]

This got a bit long... 📜 but I guess there was a lot of ground to cover...

I can't imagine why civix would need to know that mgd-php received a minor update in 5.50.

For this specific update of mgd-php, civix doesn't need to know. TBH, for mgd-php, I agree it's hard to think of a realistic case.

It's easier for me to imagine with other mixins - eg menu-xml or a hypothetical auto-listen. Let's do menu-xml:

• Suppose we decide that the file-layout should look more like core's file layout. (Consistency is good, right? Right?) That means we should load "Menu XML" files from another location, CRM/Mymodule/xml. Under SemVer, this sounds like a MINOR update (scan another file-pattern; non-breaking feature-addition; v1.1.0).
  • So we have two versions:
    • [email protected] (introduced in [email protected]): Scans ./xml/Menu/*.xml
    • [email protected] (introduced in [email protected]): Scans ./xml/Menu/*.xml and ./CRM/Mymodule/xml
  • Similarly, civix goes through two versions:
    • [email protected] depends on [email protected] and writes new files to ./xml/Menu/*.xml.
    • [email protected] depends on [email protected] and writes new files to ./CRM/Mymodule/xml (and it keeps old files inplace)
  • The mosaico#520/@since optimization would need to work like this:
    • For [email protected]: "Backport [email protected] if <compatibility> is <5.45"
    • For [email protected]: "Backport [email protected] if <compatibility> is <5.55"

I guess, in this, it doesn't really matter much if it's done as SemVer MINOR or MAJOR - either works. If we were looking at heavier mixin like auto-listen, then I'd expect more inertia pushing us toward MINOR xor MAJOR. In any event, in my mind, the mixins are defined to follow SemVer which means that MAJOR, MINOR, and PATCH increments are all fair-game.

This makes me think you have a plan in your head that I'm not privy to. ... Couldn't it just check the latest master branch of civicrm-core ...

It could check civicrm-core@master.

I suppose I have some additional constraints in mind, which we may not have fully discussed. I'll just braindump a bit...

• civix could pull mixins from several different places (e.g. github.com/civicrm/civicrm-core; e.g. the local copy of civicrm-core; e.g. a dedicated "mixin" repo; e.g. a folder inside civix). Last winter, I went back+forth on this a lot, because it's wrapped-up with QA and dev-workflow. (civix should only backport a mixin that's suitable for backporting...)
• civix actually gets metadata from a helper Mixlib. Mixlib can pull from local-files or HTTP. I think I started with HTTP first... then shifted to local-files for easier testing... and then stopped trying HTTP because some aspects were annoying.
• My back+forth stopped soon after we moved the canonical mixin store into civicrm-core.git. I settled on having civix.phar include snapshots (ie it downloads mixin/**.php files from a specific version of civicrm-core and copies them into civix.phar). Reasons...
  • It works with civicrm-core as the canonical home for most mixins. So if you follow core workflows, you get a clear path to publishing updates (regardless of what's going on in civix or in specific extensions or in old stable releases).
  • civix behavior is more clearly defined this way -- it's easier to test+run during development, it works offline, and I have more trust that the final release will work consistently. An external feed is a wildcard.
  • I'm uncertain about how long mixins will sustain backward-portability. Will [email protected] still run on [email protected] and [email protected]? Maybe? I hope so. We can encourage it. But I'm not sure. The test-regime on civicrm-core:mixin/ only runs against one branch (eg master). In 12 months time, I don't expect core reviewers looking at core@master to think about [email protected] and [email protected].
  • Assessing "backward-portability" is an unusual+fiddly QA responsibility. With this approach, that responsibility goes to a place that I think is more appropriate (or, at least, less awkward). Compare:
    • If we use a mixin live-feed (pulled from Github's civicrm-core@master): The burden of assessing backward-portability falls on the civicrm-core reviewer. But core reviewers aren't accustomed to this - the norm is to ensure that master(HEAD) is internally-consistent. And CI won't flag backward-portability issues. (Note: If one merges a mixin-patch that only works on master(HEAD)... that would break civix for all users who aren't running master(HEAD)...)
    • If we use mixin snapshots (pulled from civix.phar, per civix:composer.json): The burden of assessing backward-portability falls on the civix reviewer. If core reviewers accept some newer php74 syntax into [email protected], that's OK. If+when civix needs to use a new feature in [email protected], that will be the decisive time to ask about backward-portability.
• If the canonical home for mixin code was a separate repo (civicrm-mixin.git?), then I could see a different test-script which loops over multiple versions of civicrm-core. Then I would trust civicrm-mixin.git as an evergreen/live feed. But this comes with its own quirks, which is why went with civicrm-core.git:mixin/...

All those conditionals+contingencies makes it sound complicated, but (in my mind) the rule of thumb should be simple (though I haven't tried to write it down). Like this:

• You can update civicrm-core:mixin/ in its master branch (or RC or whatever). CI will test to ensure it works on the same-version. It will eventually be released+deployed in that version, along the usual channels (master/rc/stable/etc).
• If a mixin would allow civix to generate better code, then update civix to include a backport of the mixin. The review here may consider support on older stable releases (probably the last ESR).

---

This discussion is reminding me about why civix.git has its own list of mixins to download... maybe it's better to tweak that list with a little extra information, eg

• Move the list from civix.git:composer.json to civix.git:mixin-list.php. Add bit more info (eg the hint that says "we don't need to backport this on v5.50"). Add a step to the build-process to download.

  return [
    [
      'src' => 'https://raw.githubusercontent.com/civicrm/civicrm-core/5.50/mixin/mgd-php@1/mixin.php',
      'version' => '1.1.0', 
      'included_since' => '5.50',
    ],
  ];

• That relaxes things for core.git:mixin/**.php. We don't need to update the 5.45 branch. We can use @since tags in the way you described earlier (similar to other files that have @since).

[colemanw]

Interesting, I hadn't imagined a scenario where a new minor version of a mixin would cause a non-backward-compatible break. But in your example the [email protected] scans a new directory, which means extensions placing their menu items in that new location would be incompatible with [email protected] and would need to ship their own version of that mixin. That really sounds like a MAJOR version to me.

@totten am I missing something or are you perhaps over-engineering this solution :)
A lot of what you're proposing is yagni if we just avoid making breaking changes in minor versions of mixins. We can just keep the @since version the same across all minor versions of the mixin, and civix doesn't have to know or care which minor version is going to be supplied by core, any version will work.

[totten]

We chatted a bit by phone (yesterday). I scribbled some take-aways and wanted to cleanup/post for anyone watching the queue:

• We'll interpret @since as the version when functionality was generally introduced. ((NFC) mixin/mgd-php@1 - Twiddle @since #23440)
• We generally want to keep mixins thin+stable so that we don't put much pressure on compatibility issues.
• "Compatibility" is ambiguous. We might give tighter names to two different phenomena:
  • API Compatibility: What we normally think about. What SemVer cares about. To wit: "If my add-on used the old API, will I still be happy with the next API?"
  • File Backportability: A mixin has an extra question which arises because one may copy a single file backward. To wit: "If I copy this PHP file from a newer platform to an older platform, will it still work?"
• Examples
  • Suppose v5.35.0 added an entity PledgePayment to the API. It did not remove any entities. 5.35.0 is API-compatible with 5.34.0.
  • Suppose you refactor a core-mixin, moving some code to a new BAO helper method. This change preserves API compatibility, but it breaks file portability. This means:
    • Because it preserved API compat, you only need to bump the MINOR or PATCH version.
    • Because it broke file-backports, civix would probably not distribute this version.
• Comments
  • In the workflows for civicrm-core, we are acclimated to considering API compatibility, but file backportability is unfamiliar and difficult to enforce (even if it is nice-to-have).
  • File-backports are not performed in civicrm-core. That's a separate workflow, mediated by civix and extension-devs.
  • I think there is a natural split in these responsibilities:
    • civicrm-core should focus on API compatibility and set the version-numbers accordingly.
    • civix should focus on file backportability (and it is not obligated to do backports; that's a case-by-case / cost-vs-benefit question).