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 Product Name Metadata to Mage-OS #22

Merged
merged 2 commits into from
Oct 7, 2023

Conversation

pykettk
Copy link
Contributor

@pykettk pykettk commented Dec 14, 2022

Description (*)

Update the product name metadata from Magento to Mage-OS.

Manual testing scenarios (*)

  1. Navigate to the magento_version route
  2. Observe the version is Mage-OS/*

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)

@pykettk pykettk marked this pull request as ready for review December 14, 2022 00:14
@pykettk pykettk requested a review from a team as a code owner December 14, 2022 00:14
@rhoerr
Copy link
Contributor

rhoerr commented Dec 14, 2022

For the record: Magento and Adobe Commerce both use:
const PRODUCT_NAME = 'Magento';
They instead vary EDITION_NAME, with Magento having value Community, and Adobe Commerce having Enterprise. We would hold edition name 'Community' for parity with Magento.

Since the value of PRODUCT_NAME is always Magento in the existing products, I think it is unlikely that anyone references this constant for platform identification. This should be safe to change without major consequences.

@Vinai
Copy link
Contributor

Vinai commented Dec 21, 2022

Note:
Tests don't seem to refer to the changed name as a constant.

  • \Magento\Version\Controller\Index\IndexTest::testIndexAction checks if the value of getName is present in the response body
  • \Magento\Webapi\JsonGenerationFromDataObjectTest uses the value of getName as part of the expected JSON
  • \Magento\Framework\App\Test\Unit\ProductMetadataTest::testGetName only checks if the value is not empty
  • \Magento\Dhl\Test\Unit\Model\CarrierTest::testBuildSoftwareName uses a mock to provide the value

@Vinai
Copy link
Contributor

Vinai commented Dec 21, 2022

The Magento2 coding standard violations are present also in the upstream version of the file.
Should we fix them in one of the following ways?
A) by adding the appropriate comments to disable them
B) by addressing the complaints? Will adding a visibility of public to the constants break any descendants overriding the constant? A quick search didn't reveal and new implementation for the methods annotated with @deprecation. Do we add a stub comment to satisfy the coding standard?

@sprankhub
Copy link
Contributor

So Magento currently does not run phpcs on lib code at least!? 🤔

@pykettk
Copy link
Contributor Author

pykettk commented Dec 30, 2022

I think I'd lean towards option A at the moment, given the infancy of the project and the seeming desire to want to stay as close to 1:1 with the upstream version as possible - at least in the near term. That being said, I have no real preference, personally - I'm happy to go with whatever the general consensus is 🙂

@Vinai
Copy link
Contributor

Vinai commented Jan 15, 2023

I remember seeing somewhere that in upstream phpcs is run with --severity=7, but I don't remember where.
I've now spent an hour looking without success. I might be mistaken...
That would explain however why the core files don't trigger the warnings we are getting here.
Ref: Filtering Errors and Warnings Based on Severity.

@Vinai
Copy link
Contributor

Vinai commented Jan 16, 2023

@sprankhub pointed out this marketplace guide where extensions are checked with error-severity 10 only.

@Vinai
Copy link
Contributor

Vinai commented Jan 22, 2023

Once graycoreio/github-actions-magento2#130 is merged, the severity can be configured to 10 and we can move this PR forward.

@Vinai Vinai merged commit 17b6d76 into mage-os:2.4-develop Oct 7, 2023
1 of 3 checks passed
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.

4 participants