Modules sort order in config.php is being inconsistent #16116

[comdiler]

Description

Modules sort order in config.php is being inconsistent when no changes being made and running setup:upgrade

Fixed Issues

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. Install any Magento 2.*;
2. Run php bin/magento setup:upgrade;
3. Run cp app/etc/config.php app/etc/config.php_t1;
4. Run php bin/magento setup:upgrade again;
5. Run cp app/etc/config.php app/etc/config.php_t2;
6. Run diff app/etc/config.php_t1 app/etc/config.php_t2;
7. Check is there is/is no diff.

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-cicd2]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

[magento-engcom-team]

Hi @comdiler. 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 Pull Request changes
• @magento-engcom-team give me new test instance - deploy NEW test instance based on Pull Request changes
• @magento-engcom-team give me {$VERSION} instance - deploy Vanilla Magento instance for Issue or Pull Request

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

[orlangur]

To me it looks like this change breaks order of modules declared by <sequence> but let's wait for teh builds.

[comdiler]

@orlangur I do pre-sorting of sequence modules for each module before main bubble sorting.
Without this pre-sorting sequence modules are being loaded as they are defined in xmls (not sorted). It creates too many unexpected VCS conflicts when working in team.

[orlangur]

@comdiler I know the story behind, this was reported earlier in #8479.

No objections in merging if it really makes order more stable and does not break anything. app/etc/config.php is not a problem for merging actually just like composer.lock: you're not supposed to edit it manually.

[comdiler]

@orlangur got it, thank you. Yeah, editing it manually would be strange... We just keep it in repo just like composer.lock in order to keep same system state team-wide.
The thing is that setup:upgrade modifies it even when it's not needed. And when it's needed (sequence change or module install) logic of changes doesn't make sense. With this change logic of modification kinda make sense when making pull request reviews.

[orlangur]

@comdiler got it! I don't remember if it behaved like this before, AFAIR old case was about instability only when you add/remove module.

[sidolov]

Hi @comdiler , please, sign CLA, otherwise, we can't process your pull request

[hostep]

Sorry for intruding in here, but I have some interest in this subject and I have some questions/remarks:

@comdiler: I'm not able to reproduce your steps, am I missing something obvious here? @orlangur's comment is correct, the order only changes when you add/remove a module, not every time you run bin/magento setup:upgrade (just re-tested on a clean Magento OS 2.2.5 to be sure, PHP used: 7.0.31).

Also: by applying your suggestions to a couple of my projects with a whole bunch of 3rd party modules installed also doesn't change anything in the config.php file. I would have expected that the order would have changed in a way that the modules with the same weight would have been ordered in a deterministic way, but that doesn't seem to be the case.
But maybe I'm not understanding the purpose of this PR very well then?

So I'm a bit confused, since Magento people both in this PR as in the issue you refer to seem to confirm that the issue is happening, but I fail to see it ...

[sidolov]

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