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

Make the module list more deterministic #21020

Merged
merged 2 commits into from
Feb 24, 2019
Merged

Make the module list more deterministic #21020

merged 2 commits into from
Feb 24, 2019

Conversation

ajardin
Copy link
Member

@ajardin ajardin commented Feb 6, 2019

I've found that the modules ranking in the app/etc/config is not completely deterministic as a module addition will affect the ranking of unrelated modules. This generates a lot of conflicts when starting a Magento project since many modules are likely to be added.

Description (*)

I tried the changes proposed in #16120 at first, but the implementation was likely incomplete as I was still able to reproduce the issue. So, I came to the conclusion that we need to sort the sequence of each module AND to put all Magento modules at the top of the list before performing the bubble sorting. There is no impact on the prioritization process itself. However by keeping third-party modules together, we will mitigate the impact of a module addition because it will be naturally added at the end of the list without messing Magento modules ranking. I've also added a specific unit test.

Fixed Issues (if relevant)

  1. Modules sort order in config.php is being inconsistent when no changes being made #16116: Modules sort order in config.php is being inconsistent when no changes being made
  2. Sequence of module load order should be deterministic #8479: Sequence of module load order should be deterministic

Manual testing scenarios (*)

  1. Perform a setup:upgrade.
  2. Commit the app/etc/config.php file.
  3. Create a new module which depends of another module.
  4. Perform a new setup:upgrade.
  5. The new module is added in the app/etc/config.php file, but the ranking of several unrelated modules is also affected.

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 @ajardin. 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.3-develop instance - deploy vanilla Magento instance

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

Copy link
Contributor

@vpodorozh vpodorozh left a comment

Choose a reason for hiding this comment

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

Checked test cases:

  • standalone 3rd party module
  • three 3rd party modules with mutual dependencies
  • Magento module that had a dependency on 3rd party (including prev test cases)

@magento-engcom-team
Copy link
Contributor

Hi @vpodorozh, thank you for the review.
ENGCOM-4223 has been created to process this Pull Request

@ghost
Copy link

ghost commented Feb 24, 2019

Hi @ajardin, 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.

@scottsb
Copy link
Member

scottsb commented Feb 24, 2019

Thanks @ajardin!

foreach ($origList as $moduleName => $value) {
foreach (array_keys($modules) as $moduleName) {
$sequence = $this->expandSequence($origList, $moduleName);
asort($sequence);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make more sense to add this call into

$result = array_unique(array_merge($result, $relatedResult));

@orlangur
Copy link
Contributor

However by keeping third-party modules together, we will mitigate the impact of a module addition because it will be naturally added at the end of the list without messing Magento modules ranking.

Great, @ajardin, as I pointed out in #8479 (comment) there is no such thing as "deterministic" in general, having big enough amount of third-party modules the same issue will be observed after the fix. However, in reality there are much more core modules than external ones, thus generally diffs should become cleaner.

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.

6 participants