Add entity-types-php mixin

[colemanw]

Overview

This adds a new mixin to scan for entity types.

Before

Entity types are pre-scanned and compiled by civix into an extension's .civix.php file.

After

Now there's a new mixin and civix could be adapted to take advantage of it.

[civibot]

(Standard links)

• 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 What would be required to get civix to take advantage of this mixin? Currently civix writes the contents of every *.entityType.php directly into the .civix.php file and we'd want it to rip that out and use the mixin/shim instead as part of civix upgrade.

Edit: Hmm, now I see this comment in civix that suggests it may not be so easy:

 * In the future, it would be great to remove this delegate and use a mixin.
 *
 * However, `hook_civicrm_entityTypes` is a very unusual hook that can fire during bootstrap,
 * and it may not be amenable. We'll support it as a hook-delegation-stub until an alternative
 * is available.

@totten what do you think the boot-loading issue would be? Currently the delegated hook works, which means that extensions are all loaded when the hook is called. Wouldn't that mean mixins are loaded as well?

[eileenmcnaughton]

Could we use this as the chance for a new hook which is loaded post-boot. I feel like having a second hook for 'most' entities might help us regain some sanity

[totten]

If it's still an issue, then I'd also consider a second hook.

However, I'm not sure it's a still an issue. For a while, it was a bit of a pet-peeve for me -- because I found it rather confusing how hook_entityTypes would fire at weird moments. However, there are two encouraging signs:

• (Empirically) If you wrote this and did some r-run, then that suggests it's working. We might want to r-run form a few more angles (different environments/flavors of booting), but it's encouraging.
• (Analytically) Now-a-days, CRM_Core_Config::singleton() and Container::boot() start the system with a limited dispatch policy which should throw errors if hook_entityTypes still ran early. I did a quick breakpoint on a new build and found that hook_entityTypes executed after the container was fully bootstrapped. (The hook should run when there's first reflective operation on a DAO class -- and the breakpoint shows it running as part of this step -- which clearly comes after the container booted.)

One risk to think about - if an extension taps into hook_container and does some kind of reflective/DAO work, then it might fire a bit sooner. But... I think that would still be OK ... because \CRM_Extension_System::singleton()->getMixinLoader()->run(); still runs before hook_container.

[colemanw]

Update: I've tried this out locally by switching SearchKit to use the mixin, and everything seems to work fine. Pushed it to this PR so let's see how the tests like it...

[totten]

I've been trying this mixin with a couple other extensions and different versions of civicrm-core. The other extensions were mosaico and civigeometry; in both cases, I made a local-only branch for the ext. (In both cases, running civix upgrade and then manually doing something like 8de7881.)

With mosaico, it generally seemed alright - because the minimum version was core@5.47.

With civigeometry, the minimum was a more interesting 5.13. I didn't test that far back -- 5.27 was the test-target for some other mixins, so I tried that, but it didn't work, so I shifted further ahead to figure out the dividing line. Eventually I got more disciplined and used some throw-away scripts to try various flows and various versions.

OUTCOME         COMMAND

fail            bash tmp/try-civigeo.sh new 5.38 ; echo "exit=$?"
fail            bash tmp/try-civigeo.sh new 5.43 ; echo "exit=$?"
fail            bash tmp/try-civigeo.sh new 5.44 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh new 5.45 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh new 5.47 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh new 5.48 ; echo "exit=$?"

fail            bash tmp/try-civigeo.sh quick-up 5.44 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh quick-up 5.45 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh quick-up 5.48 ; echo "exit=$?"

fail            bash tmp/try-civigeo.sh flush-up 5.44 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh flush-up 5.45 ; echo "exit=$?"
ok              bash tmp/try-civigeo.sh flush-up 5.48 ; echo "exit=$?"

So (based on light testing) entity-types-php does not work on core <= v5.44, but it does work on core >= v5.45.

(Anecdotally, I thought I saw more interesting edges where there were momentary upgrade-failures circa 5.43-5.47, but they only appeared in some adhoc runs. When I used the more disciplined script, then that went away. It could've been procedural inconsistency. That's why I switched to testing via script.)

It's fair to add entity-types-php to core even if it doesn't run on ancient versions. However, there's the question of how civix incorporates it. Spit-balling:

• Wait: Leave civix as-is for a bit longer.
• Raise Minimum: If you generate (or update) an ext with entities, then it must target <compatibility> at 5.45 or greater.
• Opt-in: If a developer opts into civix mixin --enable=entity-types-php, then civix can stop generating the hook_entityTypes stub. Otherwise, it continues generating hook_entityTypes.

[colemanw]

@totten it sounds like the bottom line for this PR is that you should hit the Merge button. And then we can continue discussion on the civix side.

[totten]

Added more test coverage. Merge on pass.