-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add entity-types-php@2 #343
Conversation
mixin-backports.php
Outdated
'remote' => 'https://raw.githubusercontent.com/civicrm/civicrm-core/5d5c59c9453fabf1c3f709c7b8e26c680312d9a8/mixin/entity-types-php@2/mixin.php', | ||
'local' => 'extern/mixin/entity-types-php@2/mixin.php', | ||
'provided-by' => '5.73', | ||
'minimum' => '5.45', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we temporarily mark this as minimum=>5.73
? I guess we're still shaking-out that detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me! I'm still struggling to see the value of backporting this change.
18520bb
to
afea039
Compare
@totten updated the minimum. |
Ah, erg. E2E\MixinMgmtTest.testEnableAllDisableAll ==> it makes sense for that assertion to fail... The specific number is a bit arbitrary, but it would have to bounce around with this. TBH, I'm not really sure the benefit to separating this commit from the other parts of the project... (Like I view it opportunistically - if separating makes it easier, then well-and-good.) |
civibot, test this please |
@totten FIFY |
Yeah, fair enough. There's other plumbing for the overall set of changes/expectations, but that needs to be sorted out in other PRs. Don't need to block this part. |
Adds new mixin from civicrm/civicrm-core#29771