(dev/core#4999) Support Entity Framework v2 for extensions

[totten]

Overview (Final)

Implement support for Entity Framework v2 (EFv2; aka dev/core#4999). This is a general overhaul of everything related to generate:entity. There's a more complete comparison of changes at: https://github.com/totten/civix/wiki/Entity-Templates

The PR was originally opened to address one major sub-problem (installation and SQL generation). I'm preserving that description so that the discussion remains intelligible. However, the ultimate scope of the PR evolved to cover more aspects of dev/core#4999 / EFv2. The wiki page gives a more comprehensive summation.

---

(Subject: Auto-generate SQL during installation via [email protected])

Overview (Original)

For extensions that use generate:entity / generate:entity-boilerplate, this changes the SQL lifecycle:

• Remove static SQL files (auto_install.sql and auto_uninstall.sql).
• Generate dynamic SQL during installation.
• Attune the dynamic SQL to match the existing collation of the end-user's CiviCRM schema.

ping @colemanw @mattwire @artfulrobot @christianwach

Before

• civix generate:entity-boilerplate reads xml/schema/**.xml and generates SQL files (sql/auto_install.sql and sql/auto_uninstall.sql).
  • These are static SQL files. It is not possible for the SQL files to be equally correct on all deployments, because existing deployments may have different character-set/collation flags (esp utf8 vs utf8mb4).

After

• During installation, we call a new helper class CiviMix\Schema\Automatic\MyModule

  • The helper generates SQL dynamically. It matches local character-set/collation options using the same technique as CustomGroups.
  • The class is similar to a mixin. It is design to be included with civicrm-core and backported via civix. As with mixins, it will check the <compatibility> when deciding whether a backport is needed. At runtime, it will look to core-code and extension-code and choose the newest version.
  • The implementation can be seen here.

• civix generate:entity-boilerplate does not generate SQL files.

• civix upgrade will prompt you to apply the following changes

  

  Old screenshot

Related PRs

• This is an alternative to Set table collation to match core tables #329.
• The helper classes are canonically defined by (Draft) civimix-schema: Define a backportable library to handle installation schema civicrm/civicrm-core#29422.
  • This represents a soft dependency. The PR's can be merged in any order (or with delayed timing). But in the long run, one doesn't make sense without the other.
• The outcome of running this upgrade is demonstrated on mosaico.git with (Draft) Installer - Auto-generate SQL to match local schema veda-consulting-company/uk.co.vedaconsulting.mosaico#635

TODO

• For new extensions, the command generate:upgrader should set <upgrader>CiviMix\Schema\Automatic\MyModule</upgrader>.
• Need to test the generated code in more environments/versions.
• Need to test as civix.phar. (I've been testing local git copy.)
• The test suites will probably be unhappy and will probably merit some revised coverage.

[mattwire]

Nice! @totten don't let this be a blocker but just wondering if the upgrade scenario has been considered? Where a new entity is added to already installed extension and needs to install that entity on upgrade. Maybe there is a function we can call from the upgrader?

[colemanw]

Maybe there is a function we can call from the upgrader?

I agree this would be beneficial. In the upgrade scenario you can't just read from the current schema and add that table as-is, because the table might change in the future, so your upgrader has to provide a snapshot of the table structure as it first appears. And we do want to leverage the goodness of the auto-detect-collation here. So a utility function would be great that you can feed in a table definition as an array and it spits out the CREATE TABLE sql.

[totten]

Thanks, good points about upgrading @mattwire @colemanw. (Aside: I got so far into the rabbit of hole of "how to polyfill an upgrader class" that I kind of stopped thinking about the design of the upgrader-class being delivered...)

Probably the most direct thing is like you say -- add a helper function. It feels like this means switching the class-relationship.

• The current draft uses class-composition. This lets the old upgraders work untouched and hides some of the quirkiness of reloadable-classes. But it doesn't have a good way to offer new utility functions.

• Flipping to inheritance relation would allow more helpers.

  class CRM_MyModule_Upgrader extends \CiviMix\Schema\Automatic\MyModule

  But I need to think about how to do that without exposing too many quirks.

(I agree that adding the specific helper function probably shouldn't be a blocker. I'm not certain if it's easy or hard. But there should be a clear place to put that kind of helper.)

---

FWIW.... there's almost enough information to do updates like CREATE TABLE or ADD COLUMN automatically (without a named helper function) -- because the schema includes <add>NN</add> and <drop>NN</drop>. Obviously, other upgrade-steps - like filtering/modifying a column -- require richer definitions. That's why upgrade_NN() exists. Alas, the revision-sequences (the NN values) are different, and my brain would explode if <add>1.2</add> and upgrade_1002 were put in the same sequence.

I guess you could unify the sequences. Values like <add>1002</add> could be live upgrade instructions. (Equivalent to adding upgrade_1002() with an ALTER TABLE...ADD COLUMN... statement.) Values like <add>1.2</add> would have to be ignored somehow. (Ignore numbers less than 1000?...) The upshot is that you'd edit only one file for the basic case of adding new elements (without needing to copy-paste schema). And you could still define special upgrade-logic (filtering data; calling PHP code) via upgrade_NN().

I dunno, that sounds a little radical. For the moment, probably better to think about how to add helper function.

[colemanw]

... or to keep it very explicit: <autoAdd>1002</autoAdd>

[totten]

I've pushed a set of updates (but haven't updated description yet). Basically:

• Add a new helper to big E, E::schema(). It has the install() and uninstall() helpers. It's a good a place to add more utilities.
• Rename CiviMix\Schema\Automatic\MyModule to CiviMix\Schema\MyModule\AutomaticUpgrader
• In the future, we should look to update AutomaticUpgrader to include support for <add> or <autoAdd> metadata. For now, we impose some limits on AutomaticUpgrader to ensure that we will be able to slot-in extra revision-steps. (Basically, as long as downstreams strictly use the upgrade_NN() notation, we should be able to inject some more NNs. But if the structure is customized... which is extremely rare... then it would be hard.)
• Re-word the notice. Basically, the E::schema() helper is always going to be there (so civimix-schema@5 will always be required). The only decision is whether you want to enable AutomaticUpgrader.
• Updated the related PR's (for civicrm-core and mosaico) accordingly.
• Take a pass at fixing some of the unit-tests. (There are probably still more issues, but I think got a few.)

[totten]

civibot, test this please

[totten]

So I rebased and switched this from the unreleased [email protected] (XML-flavor schema helper) to the merged [email protected] (PHP-flavor schema helper). Quick take:

• Everything that was in the scope of the PR is... still doing what it was supposed to (i.e. it uses metadata to generate the install/uninstall SQL at runtime). 🟢
• But goal posts move on other aspects - e.g. one needs to also address XML=>PHP conversion and entity-type-php@1 => entity-type-php@2. ⚠️
• To demonstrate on mosaico, I did the automatic upgrade and then manually updated those other bits. (Branch) And it does create new tables. 🎉

[colemanw]

@totten what's the status of this?

[colemanw]

@totten I've just finished converting the rest of our core extensions to use this.

[totten]

Ah, blerg. There's a test-interaction problem (i.e. tests pass individually but fail in bulk). E2E tests look like this:

   $this->civixGenerateModule(static::getKey(), [
      '--compatibility' => '5.69',
    ]);
    $this->civixGenerateUpgrader();
    $this->civixGenerateEntity('Bread');
    $this->civixGenerateEntity('Sandwich');

• These steps actually run in the same PHP process. This is good for debugging/inspection -- but bad for fidelity. In real usage, they run as separate PHP processes.
• If you run several tests in the same process, then the relevant files/functions only load once. (The first variant of function xyz() {...} stays in memory.)
• This hasn't been a problem before because...
  • The code-generators were traditionally/typically additive (each test/step makes a new extension/file/function/class).
  • The code-generators were traditionally/typically passive (they don't necessarily boot CiviCRM; they just fiddle with strings).
• But now...
  • It's starting to lean more on conditional construction (and regeneration) of *.civix.php. Steps aren't just additive -- they can change the content of *.civix.php. (This PR is a good example.)
  • It's starting to incorporate more active functionality from CiviCRM -- so some steps require booting (while other steps don't).
• The overall effect is chaotic/painful to trace.
• What's probably needed is to change the test-structure to improve isolation. We have a couple high-leverage points:
  • phpunit's --process-isolation puts each test-case in its own process. But it's not quite realistic -- individual steps would still share the same process.
  • CivixProjectTestTrait::civix() calls Symfony's CommandTester. Swap that with something similar that uses sub-processes. This could reveal some other hidden interactions, but it's probably better in the long run.

tldr: I need a separate PR to change the testing process.

[totten]

Fixed a "WIP" commit that was outstanding. We had a clean test-run. I also did some local trials like this:

• Install 5.45. Make extension foo545. Add entity Foo545. Install this anew on 5.45 (and also 5.69 and 5.78).
• Install 5.69. Make extension foo569. Add entity Foo569. Install this anew on 5.69 (and also 5.78).
• Install 5.78. Make extension foo578. Add entity Foo569. Install this anew on 5.78.

I also went back and re-tried civix upgrade and civix convert-entity on mosaico.git (with [email protected]). Ran into a couple issues. In my basic testing, the latest couple commits deal with these. civix upgrade will prod you over to EFv2.

Hopefully (knocks on wood) test-suites will still be happy...

[totten]

FIXME: Upgrade/convert step also needs to generate skinny DAOs.

[totten]

The upgrade/convert step will now re-generate DAOs -- and delete the old XML schema files.

Unless the test-suite has some complaints with this revision, then I think the PR may be ready to merge....

[colemanw]

Looks good @totten

As you probably remember, my vision is for these thin DAOs to be temporary, and as the core EntityRepository matures, the API will switch to calling $entity->writeRecord() directly and the DAO itself will be obsolete. But I think this is good for now.