Skip to content

Conversation

@colemanw
Copy link
Contributor

@totten
Copy link
Owner

totten commented Sep 17, 2023

Well... the tests have issues...

@colemanw
Copy link
Contributor Author

@totten we're still getting minor versions written to xml files and it's quite confusing. For example, I just tried running civix upgrade on an extension, and civix balked at this line in the extension's info:

<mixin>scan-classes@1.1.0</mixin>

image

It's not my extension so I'm not sure why the line was that way, but probably because somebody wrote it manually, based on the logical assumption that the version ought to match the one in core. Currently that is the correct version:

 * @mixinName scan-classes
 * @mixinVersion 1.1.0

so I can't fault the author for putting that in, (but civix does).

I think all this confusion could be avoided by switching civix to only refer to major versions of mixins. You said tests were a blocker here. Can you help me get those fixed?

@colemanw
Copy link
Contributor Author

@totten ?

@colemanw
Copy link
Contributor Author

@totten this is still an issue.

@totten
Copy link
Owner

totten commented Oct 29, 2025

civibot, test this please

@totten
Copy link
Owner

totten commented Oct 29, 2025

probably because somebody wrote it manually, based on the logical assumption that the version ought to match the one in core

Yeah, I can see how someone might assume that, but the assumption is wrong. 👀

  • It's a bit like saying: "somebody wrote a requirement for ECMAScript 2025 because that's the version in the latest Firefox". Yes, on rare occasion, one might make that inference.
  • However, if you're a publisher with an open audience, then it's better practice to choose the lowest version that you find acceptable for reaching your audience (say, ECMAScript 2020) -- and set that as the requirement. The latest Firefox might support ES2025, and some of your users might have ES2023. But if ES2020 does the job, then use that.

You said tests were a blocker here. Can you help me get those fixed?

OK, re-ran tests. It still fails. I believe the test failures are saying something like this: "If you only specify major-version, then it stops writing proper+consistent backport files." For example... IdempotentUpgradeTest ensures that you can safely run civix upgrade several times.

  • In testBasicUpgrade(), it creates an extension with an entity and then runs civix upgrade (with no real steps to run; just a pro-forma thing).
  • Initially, the generated extension includes a copy of mixin/entity-types-php@2.0.0.mixin.php.
  • But, after civix upgrade, the mixin file disappears.

One could probably update the thing which decides whether to write the *.mixin.php backport file. I have a nagging sense that this would open up more edge-cases.

It should be said: civix's current mixin-handler was not designed to support a free-for-all of mixin-version-constraints. It works with an opinionated list with a known/preferred versions, and it deals with tangential requirements (like optimizing-out *.mixin.php files). For the oblivious user (who runs civix upgrade and tunes-out mixin versions), I suspect that declaring @X.Y.Z is actually a better play than @X. (There's a simplicity/predictability in having the info.xml, mixin/*.mixin.php, and civix policies all match.)

But anyway, back to the issue -- that error message is annoying (for the user who tries to manually tune the versions). Some other options for tackling that:

  1. Rephrase the error. Explain that civix has a few preferred versions (mixin-backports.php). You can do manual edits in a pinch, but civix won't be able to help with maintenance.
    • (Aside: If someone thinks it's important to use a newer version of the mixin, then we should probably update mixin-backports.php to prefer that version... and then the error goes away...)
  2. Support arbitrary mixin-version-constraints. Create a real feed of mixin versions. (This basically requires analyzing the annotations and git-tags for civicrm-core.git:mixin/.) Re-work civix mixin (et al) to use that feed (and work-through the edge-cases).

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