Open totten opened 6 months ago
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?
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.
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.
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.
... or to keep it very explicit: <autoAdd>1002</autoAdd>
I've pushed a set of updates (but haven't updated description yet). Basically:
E::schema()
. It has the install()
and uninstall()
helpers. It's a good a place to add more utilities.CiviMix\Schema\Automatic\MyModule
to CiviMix\Schema\MyModule\AutomaticUpgrader
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 NN
s. But if the structure is customized... which is extremely rare... then it would be hard.)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
.civicrm-core
and mosaico
) accordingly.civibot, test this please
So I rebased and switched this from the unreleased civimix-schema@5.72
(XML-flavor schema helper) to the merged civimix-schema@5.74
(PHP-flavor schema helper). Quick take:
entity-type-php@1 => entity-type-php@2
. ⚠️ mosaico
, I did the automatic upgrade and then manually updated those other bits. (Branch) And it does create new tables. 🎉 @totten what's the status of this?
@totten I've just finished converting the rest of our core extensions to use this.
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');
function xyz() {...}
stays in memory.)*.civix.php
. Steps aren't just additive -- they can change the content of *.civix.php
. (This PR is a good example.)--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 this is looking good!
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-TemplatesThe 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
civimix-schema@5.72
)Overview (Original)
For extensions that use
generate:entity
/generate:entity-boilerplate
, this changes the SQL lifecycle:auto_install.sql
andauto_uninstall.sql
).ping @colemanw @mattwire @artfulrobot @christianwach
Before
civix generate:entity-boilerplate
readsxml/schema/**.xml
and generates SQL files (sql/auto_install.sql
andsql/auto_uninstall.sql
).utf8
vsutf8mb4
).After
CiviMix\Schema\Automatic\MyModule
CustomGroup
s.civicrm-core
and backported viacivix
. 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.civix generate:entity-boilerplate
does not generate SQL files.civix upgrade
will prompt you apply the following changesRelated PRs
mosaico.git
with https://github.com/veda-consulting-company/uk.co.vedaconsulting.mosaico/pull/635TODO
generate:upgrader
should set<upgrader>CiviMix\Schema\Automatic\MyModule</upgrader>
.civix.phar
. (I've been testing local git copy.)