Skip to content
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

Update civix templates #520

Merged
merged 3 commits into from
May 22, 2022
Merged

Conversation

totten
Copy link
Collaborator

@totten totten commented May 3, 2022

@mattwire
Copy link
Collaborator

mattwire commented May 3, 2022

@totten What is the minimum CiviCRM core version requirement for this?

@totten
Copy link
Collaborator Author

totten commented May 4, 2022

@mattwire I've tested the civix updates generally on 5.39, 5.44, 5.45, and master. Since mosaico's current <compatibility> points to 5.48, this shouldn't change the minimum.

(ED: And I would expect compat further back, but I haven't tested further back.)

@colemanw
Copy link
Contributor

colemanw commented May 5, 2022

@totten as you noted, Mosaico's current <compatibility> is 5.48, therefore I don't think any of the shims in this PR are needed, as they are all in core as of 5.45.

@totten
Copy link
Collaborator Author

totten commented May 10, 2022

Correct, they're not required. But I'm ambivalent. There's no runtime cost (CPU/RAM), though there is a passive-storage cost (disk-space). The practice of curating those files also has a cost -- one has to think about the compatibility-matrix and decide when/whether to commit each.

Comparing the size of the files against the curation cost... I don't think it's really worthwhile to fine-tune the list in specific extensions. I'd go for commit+forget.

Of course... from a systemic POV... things do a add-up. It be better if civix filtered based on the <compatibility>. That's probably a few hours of work though. (We'd need to add some more metadata to each mixin... eg @providedBy 5.45)

@colemanw
Copy link
Contributor

(We'd need to add some more metadata to each mixin... eg @providedBy 5.45)

I think the @since annotation is standard in PHP; it's something we use in other places like APIv4 entities.

totten added 3 commits May 20, 2022 18:54
The implementation of `hook_angularModules` had a funny guard at the top, and it becomes
inoperative when switching to `ang-php` mixin. This reinstates the filter.
@totten
Copy link
Collaborator Author

totten commented May 21, 2022

Updated again with 22.05.2. This removes the backport files.

@@ -373,6 +324,7 @@ function mosaico_civicrm_mailingTemplateTypes(&$types) {
* Implements hook_civicrm_entityTypes().
*/
function mosaico_civicrm_entityTypes(&$entityTypes) {
// _mosaico_civix_civicrm_entityTypes($entityTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary addition.

@colemanw
Copy link
Contributor

I don't have merge rights to this repo but 👍 for merging.

@mattwire mattwire merged commit 7a80e6c into veda-consulting-company:2.x May 22, 2022
@totten totten deleted the update-civix branch May 24, 2022 00:07
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.

3 participants