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

[Backport] Fix magento root package identification for metapackage installation #21137

Conversation

oleksii-lisovyi
Copy link

@oleksii-lisovyi oleksii-lisovyi commented Feb 11, 2019

Original pull request: #22116

Description (*)

Fix issue with failing Magento installing process (php bin/magento setup:install) for installation via metapackage, because of issue in root package defining.

Fixed Issues (if relevant)

  1. Magento installation via metapackage: checkExtensions fails #21136: Magento installation via metapackage: checkExtensions fails, because isMagentoRoot works incorrectly for installation

Manual testing scenarios (*)

Steps to reproduce could be takes from issue.

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)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @oleksii-lisovyi. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-engcom-team magento-engcom-team added Component: Framework/Composer USE ONLY for FRAMEWORK RELATED BUG! E.g If bug occurs with Catalog use just Catalog Release Line: 2.2 labels Feb 11, 2019
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-4211 has been created to process this Pull Request

@ihor-sviziev
Copy link
Contributor

Hi @oleksii-lisovyi,
Could you create PR with the same changes to 2.3-develop branch?

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Please squash all changes into single commit and force push your chagnes

@@ -6081,6 +6081,8 @@
"stability-flags": [],
"prefer-stable": false,
"prefer-lowest": false,
"platform": [],
"platform": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this changes, It looks incorrect

@oleksii-lisovyi oleksii-lisovyi force-pushed the fix-magento-root-package-identification-for-metapackage-installation branch from 4f43584 to 48b4707 Compare February 19, 2019 16:15
@ihor-sviziev
Copy link
Contributor

I checked integration tests failures, looks like we have some legacy test there. I'll discuss what to do with it with @buskamuza. For now no action items needed.

@magento-engcom-team magento-engcom-team added Component: Composer and removed Component: Framework/Composer USE ONLY for FRAMEWORK RELATED BUG! E.g If bug occurs with Catalog use just Catalog labels Mar 4, 2019
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-4211 has been created to process this Pull Request

@soleksii
Copy link

✔️ QA Passed

Before:

before

After:

after

@nmalevanec nmalevanec requested review from nmalevanec and removed request for nmalevanec March 27, 2019 14:24
@nmalevanec nmalevanec self-assigned this Mar 27, 2019
@nmalevanec
Copy link
Contributor

@oleksii-lisovyi, please check failed integration test Magento\Framework\Composer\ComposerInformationTest::testGetRequiredExtensions with data set "Skeleton Composer" ('testSkeleton')
Failed asserting that an array contains 'intl'.

@ihor-sviziev
Copy link
Contributor

Hi @nmalevanec,
It looks like these tests are too old and incorrect.
I wasn’t able to find you in slack. Could you write me a message and let’s discuss what to do with it?

@sivaschenko
Copy link
Member

@oleksii-lisovyi @ihor-sviziev is there a related pull request to 2.3-develop?

@oleksii-lisovyi
Copy link
Author

@sivaschenko, here it's: #22116

@orlangur orlangur self-assigned this Apr 2, 2019
@sivaschenko sivaschenko changed the title Fix magento root package identification for metapackage installation [Backport] Fix magento root package identification for metapackage installation Apr 3, 2019
@ghost ghost removed the Progress: accept label Apr 24, 2019
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @oleksii-lisovyi,

Right now backport looks really different than original PR.
Could you update this PR with changes that were done there?

Thank you!

@ghost ghost unassigned soleksii Jun 24, 2019
@sidolov
Copy link
Contributor

sidolov commented Jul 8, 2019

@oleksii-lisovyi , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Jul 8, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 8, 2019

Hi @oleksii-lisovyi, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants