Sequence of module load order should be deterministic

[scottsb]

Preconditions

Magento 2.1.3

Steps to reproduce

1. Developer enables a new module using CLI command.
2. Magento adds an entry for the module to config.php.

Expected result

The only change to config.php is the addition of the module that changed.

Actual result

In addition to the new line for the new module, many other lines change sequence.

Discussion

It seems that the current semi-official recommendation is not to version the config.php file with the code base and to treat it as an asset along with images, as evidenced by the fact that it is git-ignored in the Magento repository by default and by the fact this advice is upvoted and without alternative:
http://magento.stackexchange.com/a/130109/567

If I understand the intent behind this recommendation, it's that one should be able to pull the latest copy of the code without immediately breaking the site due to an incompatible database schema. My counterpoint is that since themes and existing modules may be modified to depend on certain new modules being enabled, pulling code without including config.php is just as likely to break a site.

Ultimately whether config.php is versioned or not is probably a decision best made by each individual implementor. However, for us, it definitely makes sense to version it. It allows us to have one atomic unit of enabled code to test and deploy from dev through to production. I know others will agree, given the comments on #800 and the original Stack Exchange question referenced above.

Given that, in order to minimize merge conflicts, it would be best for the sequence of modules in config.php to be deterministically generated. It appears that currently it is based on the module load order, but that is not fully deterministic. Since the sequence of modules in config.php is not itself used to determine any programatic behavior, my suggestion is that the array be sorted alphabetically by key, but I would consider any deterministic ordering to meet the goal.

[korostii]

While I agree that seemingly random order in config.php may lead to unexpected side-effects, and that it might be a good idea to versionize it is well (also in order to track which modules should be enabled), you have to also consider that this file is in fact re-generated by Magento whenever you enable or disable a module based on modules' tags (more on that here ) thus purely alphabetical is just not an option. (I'm sincerely sorry if I misunderstood your point, please do correct me If I missed something)

[scottsb]

@korostii, I know this file is regenerated every time you enable/disable a module. That's why I'm suggesting that the order be deterministic. That way when it's regenerated, the only change between the old version and the new version is the relevant line.

Before I created the issue I read through the code handling config.xml, and it appears to be treated by Magento only as an unordered map. The modules' own tagging (the <sequence> tags in the various module.xml files) appear to be solely what control load order. As far as I can tell there's no need for the config.xml order to correspond to the load order, so alphabetic order should be fine. (I do note that I can't quite reconcile this with the statement in the documentation stating that config.xml has to be regenerated in order for changes in the <sequence> tags to take effect).

[scottsb]

In case others find it helpful, this one-liner will show you the real changes in the config.php after it's been mangled by a new module install (assuming you're version controlling your config.php in Git and currently have the new module changes uncommitted):

diff -wB <(sort app/etc/config.php) <(git show HEAD:app/etc/config.php | sort -)

HT: http://stackoverflow.com/a/20379403/1583214

[scottsb]

I was previously incorrect about config.php not being used in determining load order. It is necessary for that: essentially working as a cache of the module order determined by the <sequence> tags. Given that, this issue is really more a request that the module load order be deterministic. Right now the module load order just resolves the requirements, but the specific order could still vary where the <sequence> requirements allow it. I will update the issue title as such.

[orlangur]

@korostii,

seemingly random order in config.php may lead to unexpected side-effects

Like which?

it might be a good idea to versionize it is well (also in order to track which modules should be enabled)

Agree, but what is the problem in lines reordering? I hope nobody is changing config.php file manually thus it is absolutely no need to review this file changes manually.

@scottsb,

The only change to config.php is the addition of the module that changed

Please describe a real-world scenario where such order does make sense. What would be the value of such behavior if it is possible to be implemented?

When we are talking about just one module added criteria is pretty easy - "put module in earliest possible spot" or "as close to tail of module list as possible". But I'm pretty confused by what do you mean by "deterministic" in common case, when multiple modules could be enabled, containing some <sequence> data or some <sequence> data is changed.

TL;DR
There is nothing to fix here besides bugs which break the specified <sequence> order of modules in some cases. Just like composer.lock, any order of resolved dependencies is valid, you should not touch it manually and you definitely can store config.php under VCS in some circumstances (it may indicate some necessary <sequence> dependencies are missing, it's always better to fix a problem than a sympthom) but there is no need in any additional criterias like lexicographic order or minimal possible diff.

From my understanding composer already have all we need to declare correct order of modules since composer/composer#655 (comment) and it would be good to deprecate <sequence> node completely in favor of composer constraints. I suggested it long time ago but not sure if it was ever considered thoroughly (Cc: @antonkril @maghamed @buskamuza was it maybe rejected somewhere around composer.json introduction for modules?). The only concern coming to my mind is that composer solver could be way too slow for 200-300 dependencies.

[scottsb]

@orlangur I agree that the config.php module declarations should not be manually modified in any normal circumstances. The main reasons in my mind for wanting this file versioned are:

1. Consistency across environments (keeping the same modules enabled or disabled).
2. Traceability (identifying when in the code base a given module was added or enabled).

Consistency does not require any specific order (short of the implicit dependencies as mentioned in #10649), but traceability does (to do it easily). Ideally I should be able to do a git blame on the config.xml file and see the last time that any module listed was changed, without seeing a lot of noise from reordering.

I'm pretty confused by what do you mean by "deterministic" in common case, when multiple modules could be enabled, containing some <sequence> data or some <sequence> data is changed.

If sequencing for modules change, I would expect to see the sequence in config.xml change.

By determinacy I mean at a minimum "stability" as discussed in #10649, but more than that. Stability only says that the order of equal keys will be preserved after sorting; determinacy (as I'm using it) would mean that even given a random initial state, the same order would always result after sorting. This would require sorting on more than just the <sequence> tags by adding some sort of comparison that can never result in equal keys. The obvious option would be to add an alphabetic comparison of module name as a secondary comparison to the sequence data (if still using bubble sort) or as a presort (if switching to a stable topological sort algorithm).

[scottsb]

Granted, in most cases true stability would be enough to keep the commit diffs on the file to a minimum. Determinacy as I described it would be ideal but only relevant in the off case that the same change is introduced by two different developers. Even that would likely introduce a git conflict, but at least it would be more obvious how to merge than if the modules ended up sorted differently.

[magento-engcom-team]

@scottsb Thank you for your submission.
We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues. Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

[magento-engcom-team]

@scottsb, thank you for your report.
This seems to be correct Magento behavior. Please refer to the Community Forums or the Magento Stack Exchange site for advice or general discussion about this.
Otherwise you may submit Pull Request with the suggested changes.

[adamzero1]

We had a similar issue, where the ordering of modules changing was causing us problems, this is the solution we came up with: <>, allowing it to change in development mode and when running anything other than setup:upgrade (as setup:upgrade is the only command that can change this and should be run on live)

[orlangur]

@adamzero1 please propose changes creating a pull request instead of posting some patch links.

[magento-engcom-team]

Hi @scottsb. Thank you for your report.
The issue has been fixed in #21020 by @ajardin in 2.3-develop branch
Related commit(s):

• 7b9c6058e8462165dd975ab9d005f671d29a5492
• 78916b3f987d47febbdf443b8839baca780e9c02

The fix will be available with the upcoming 2.3.2 release.

[magento-engcom-team]

Hi @scottsb. Thank you for your report.
The issue has been fixed in #21423 by @eduard13 in 2.2-develop branch
Related commit(s):

• abc3e7c17b11cfc99edc06495a5da47c0c82d9f0
• 4b2c1b47998775c114b443ae3dae78d532727598
• 730987f7405376c7d9cf8964ef61f74a09c2c019

The fix will be available with the upcoming 2.2.9 release.