Skip to content

Use entity-type-php mixin - #272

Merged
totten merged 5 commits into
totten:masterfrom
colemanw:entityTypePhpMixin
Feb 23, 2023
Merged

totten merged 5 commits into
totten:masterfrom
colemanw:entityTypePhpMixin

Conversation

@colemanw

Copy link
Copy Markdown
Contributor

Adds an upgrader for the new mixin. Depends on civicrm/civicrm-core#24947

@colemanw

Copy link
Copy Markdown
Contributor Author

@totten I'm still unclear on why you think this mixin might not work. See my question here: civicrm/civicrm-core#24947 (comment)

@totten

totten commented Nov 22, 2022

Copy link
Copy Markdown
Owner

(1) Noted in the other core thread -- in my testing,the mixin seemed to work with core>=5.45 but not core<=5.44. That covers two ESR releases (5.51 and 5.45), so I think it's fair for entity-generation to require 5.45.

(2) There needs to be a registration record along the lines of this (the snippet I used for testing last night):

diff --git a/mixin-backports.php b/mixin-backports.php
index b2ebdf0..918943d 100644
--- a/mixin-backports.php
+++ b/mixin-backports.php
@@ -35,6 +35,14 @@ return [
     'provided-by' => '5.45.beta1',
     'minimum' => '5.27', /* Compat may go back further; haven't tested */
   ],
+  'entity-types-php@1' => [
+    'version' => '1.0.0',
+    'sha256' => 'f8e10aac991b2b3acac269a1fca81f883a295908990687959e790885a2e410c2',
+    'remote' => 'https://raw.githubusercontent.com/civicrm/civicrm-core/8de7881b35e3f98d38b147dcf23f5a44d5dfe035/mixin/entity-types-php%401/mixin.php',
+    'local' => 'extern/mixin/entity-types-php@1/mixin.php',
+    'provided-by' => '5.57.beta1',
+    'minimum' => '5.27', /* Compat may go back further; haven't tested */
+  ],
   'menu-xml@1' => [
     'version' => '1.0.0',
     'sha256' => '4f5be44d6764816b22d0a5cdc2e047cfd9ec4acf48e548f82bb20c05db933d0e',

(That obviously needs some tweaking - e.g. bump minimum to 5.45 and maybe get a prettier remote URL.)

(3) So... how to put some force to the minimum.... the smallest patch might update \CRM\CivixBundle\Builder\Mixins::addMixinToXml() so that it revises both <mixins> and <compatibility>. The change would become apparent in git (git diff / git commit), though it would be more polite to sprinkle in some confirmations. (e.g. 22.12.0.up.php would warn that it's going to raise the <compatibility>).

@colemanw

Copy link
Copy Markdown
Contributor Author

@totten that all sounds good to me. So to recap, if I run civix generate:entity it will check core compatibility and confirm if it needs a bump to 5.45, and abort if I type n. Effectively that means civix will not generate entities for modules with older core versions, which I agree is perfectly reasonable.

@totten
totten force-pushed the entityTypePhpMixin branch from 2ce7d48 to 8bd8dfe Compare November 23, 2022 00:22
@totten

totten commented Nov 23, 2022

Copy link
Copy Markdown
Owner

Rebased on top of current master and sibling PR #274.

Comment thread tests/e2e/CivixProjectTestTrait.php Outdated
Comment thread mixin-backports.php Outdated
'entity-types-php@1' => [
'version' => '1.0.0',
'sha256' => 'f8e10aac991b2b3acac269a1fca81f883a295908990687959e790885a2e410c2',
'remote' => 'https://raw.githubusercontent.com/civicrm/civicrm-core/8de7881b35e3f98d38b147dcf23f5a44d5dfe035/mixin/entity-types-php%401/mixin.php',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@totten so this url will be prettier once civicrm/civicrm-core#24947 is merged right? IMO let's go ahead and merge it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok merged. This URL can be updated.

@totten
totten force-pushed the entityTypePhpMixin branch 2 times, most recently from 2740eac to 17ef649 Compare December 20, 2022 00:36
@totten
totten force-pushed the entityTypePhpMixin branch from 17ef649 to 7ac0223 Compare February 23, 2023 00:09
@totten
totten force-pushed the entityTypePhpMixin branch from 7ac0223 to 559af1f Compare February 23, 2023 01:03
@totten

totten commented Feb 23, 2023

Copy link
Copy Markdown
Owner

Rebased and updated some of the numbering.

CI test run looks good in a recent run (very-similar commit) with core@master,5.57,5.51. So expecting the next run to look good.

Locally, I've also tested with core@5.45 because that's the version we marked before as the min-compat for entity-type-php. And it was passing.

We should merge after he next test-run passes.

@totten
totten merged commit d44a7cf into totten:master Feb 23, 2023
@colemanw
colemanw deleted the entityTypePhpMixin branch March 8, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants