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

Split Magento and Mage-OS versions in ProductMetadata for compatibility #115

Open
wants to merge 5 commits into
base: release/1.x
Choose a base branch
from

Conversation

rhoerr
Copy link
Contributor

@rhoerr rhoerr commented Jan 13, 2025

Description (*)

Some extensions use version comparison in Magento to determine what code to run. This is a problem for Mage-OS, where we replace Magento's getVersion() number (IE 2.4.7) with the Mage-OS version (IE 1.0.5), which makes some extensions think it's an old version of Magento and run old incompatible code.

The idea here is:

  • Magento gets the version from composer.lock for magento/product-community-edition, via https://github.com/rhoerr/mageos-magento2/blob/7bed30c9ea2b6e31130d4cead3e191871fb9b98e/lib/internal/Magento/Framework/App/ProductMetadata.php#L118
  • Mage-OS replaces that version number during our existing build/release process.
  • To fix it, we change getVersion to return the equivalent Magento version, and add a separate method to get the Mage-OS version.
  • feat: add upstream version to metapackage composer.json for runtime use generate-mirror-repo-js#189 will store the upstream version as extra.magento_version on the product metapackage composer JSON, which is stored in composer.lock and already read by Magento.
  • getVersion() (via getSystemPackageVersion()) is changed to read that upstream version (like 2.4.7-p3), for intercompatibility with Magento. That will return the equivalent version for releases going forward. Side benefit: it'll be easier to know what the equivalent version being run actually is.
  • A new getDistributionVersion() (via getSystemDistroVersion()) will return the distribution version (like 1.0.5), from the metapackage version. If any code needs to know the exact Mage-OS version, it can use this to get it.
  • All references to version number (admin footer, CLI, /magento_version, swagger) have to be changed to use getDistributionVersion() instead of getVersion().

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes MageOS version #108

Manual testing scenarios (*)

  1. Build a Mage-OS preview release (currently unable to; see related PR above).
  2. Run CLI command bin/magento --version; see output like: Mage-OS CLI 1.0.5 (based on Magento 2.4.7-p3)
  3. Open the admin panel and log in; view the footer. See output like: Mage-OS ver. 1.0.5

Questions or comments

Opening this as a draft PR due to inability to test it end to end, as the build process is currently broken.

Unit/integration tests might be affected and require further attention. I didn't get there yet.

I split getVersion(), but not getName(), which returns Mage-OS rather than Magento. Should that be split too?

I created a new interface DistributionMetadataInterface, on the thinking that third parties could use that (instanceof DistributionMetadataInterface) to help determine whether an install is Mage-OS, and that this would be better for backwards compatibility by not introducing new methods to ProductMetadataInterface. Thoughts?

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I like the fact that the original getVersion() goes back to return the magento version. I see that there's a getDistributionVersion(), is there also a getDistributionName()?

lib/internal/Magento/Framework/App/ProductMetadata.php Outdated Show resolved Hide resolved
lib/internal/Magento/Framework/App/ProductMetadata.php Outdated Show resolved Hide resolved
lib/internal/Magento/Framework/App/ProductMetadata.php Outdated Show resolved Hide resolved
@rhoerr
Copy link
Contributor Author

rhoerr commented Jan 14, 2025

Code changes, for further consideration.

I was able to create a test build with the composer data, so I can try the actual UX aspect now (maybe later this week).

@rhoerr
Copy link
Contributor Author

rhoerr commented Jan 15, 2025

Tested end-to-end with generated packages from the associated PR.

Admin footer:
image

CLI:

$ bin/magento -V
Mage-OS CLI 1.0.5-p24 (based on Magento 2.4.7-p3)

@rhoerr rhoerr marked this pull request as ready for review January 15, 2025 02:06
@rhoerr rhoerr requested a review from a team as a code owner January 15, 2025 02:06
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I like this a lot

@rhoerr
Copy link
Contributor Author

rhoerr commented Jan 19, 2025

I fixed unit test errors, those pass now.

Integration tests keep failing for unrelated process errors. I'm not sure if there are any issues caused by these changes.

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