ConfigImportUITest::testImport fails when the module list changes

Are you sure that is actually random? Did it happen locally or on a testbot?

I had that as well locally and I've also had troubles with similar tests in DependencyTest.

Somehow the order is different for me locally, the only way to get that to pass (It was in the issue that made node.module optional I think) was to go after the order that the testbots were getting and ignoring local test fails.

We need to fix it either way, just trying to identify the actual problem. I was wondering if it is 5.5, but alexpott is also running 5.5 and he wrote those tests...

Strange. Wondering if we can somehow identify the root cause, must be something about our graph/dependency code? Maybe the results aren't stable under a certain scenario?

When I did see this in DependencyTest, which involves very similar modules, it was always returning a different order for me.

Based on the discussion in IRC, having more or less modules, (in /modules but also in core, as this issue is adding a new one), can affect the order of modules.

While very unfortunate, I think we can just change the order in #2208617: Add key value entity storage (hint, you referenced the wrong issue) and move on.

We should still looking into improving this however, and possibly make the test use modules that have a direct dependency chain within themself so that the order remains stable? We're right now testing if two modules without any relation to each other (or any other module, because neither of them have any dependencies or have anything depending on them) are installed in the same order, but given their dependency situation, that order is really irrelevant?

So this is blocking #1874640: Rename edit module to quickedit then, which is a beta target.

Patch makes the module order for uninstall and install during config sync totally predictable. Sorts by the dependency order and then the module name. The sorts ensure that install and uninstall are complete opposites. See https://stackoverflow.com/questions/2282013/php-array-multiple-sort-by-v...

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -373,13 +373,15 @@ protected function createExtensionChangelist() {
   +    // Sort the module list by their weights. So that dependencies are
   +    // uninstalled last. Modules of the same weight are sorted in reverse
   +    // alphabetical order.
   ...
   +    // are installed first. Modules of the same weight are sorted in
   +    // alphabetical order.
   

   Definitely +1 to adding a comment here. However, let's be more explicit so it's more clear how and why we are using array_multisort(). Reading this, I wouldn't know not to switch back to an arsort(). @sun suggested expanding the comment to more clearly document the intended sorting algorithm, and linked ExtensionDiscovery.

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
   @@ -152,8 +152,8 @@ function testImport() {
   -    $expected = array('ban', 'action', 'text', 'options');
   -    $this->assertIdentical($expected, $installed, 'Ban, Action, Text and Options modules installed in the correct order.');
   +    $expected = array('action', 'ban', 'text', 'options');
   +    $this->assertIdentical($expected, $installed, 'Action, Ban, Text and Options modules installed in the correct order.');
   

   How does this order change guarantee that we won't break this again in the future? It (and the PHP ordering weirdness) are specific to the set of modules on the particular Drupal site.

As written above, I think we should be using modules that have direct dependencies between them.

We're testing an order where there is none, action and ban have no relation to each other and there is no need for them to be in a specific order, that's why any change in the module list can affect their order.

1. Fixed - added improved docs and move the sorts to protected methods.
2. Not sure how to add a more specific test for this. Essentially we'd need to test that we don't go back to this http://3v4l.org/9suEd but since we don't controller the number of modules Drupal has I'm at a loss.

Re #12 I think it is valuable to have a known order that modules of the same weight will be enabled during a config sync since that might help us catch crazy things - or never see crazy things - but what is less likely to happen is that one one person has a crazy thing happen that is unrepeatable.

This addresses #1 about the comments by making the sorting its own methods. Just an idea about how we can make this array manipulation less terrifying, less regression-prone, and more self-documenting, though honestly it might be better to factor out more of it.

Still thinking about #2.

lols.

I'll combine these two.

No this.

Also, now we have unit-testable methods. ;) Would address part of my concern in #2. Though the thought about more refactoring of the array manipulation still stands.

Unless there is a technical use-case/requirement, please remove the separate methods.

Functions and methods exist for re-use of code logic, NOT to document logic. Use inline comments to document non-obvious code logic.

Usage of array_multisort() is perfectly fine (+ congratulations for figuring out how it works :-))

However, every usage of array_multisort() should always be preceded with an inline comment that explains the (intended) sorting algorithm, which, in essence, is a ORDER BY on multiple columns in a multi-row + multi-column result set.

Examples:

http://drupalcode.org/project/drupal.git/blob/HEAD:/core/lib/Drupal/Core...
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/includes/module...

Also. We can just array_reverse() for more reliable reliability. Ahem.

@sun, frankly, plenty of people will disagree with you.

@xjm, I don't know why you disagree with properly documenting complex math constructs.

array_multisort() is nothing else than a math algorithm natively provided by PHP core. It's pointless to wrap each call to it into a separate method for documentation purposes only. Based on that logic, you could as well wrap each all to array_diff_key(). No, thanks.

@sun, you misunderstood my comment. I am absolutely in favor of thoroughly documenting the sort order, and that's what both #13 and #14 try to do and what I'm finishing now.

@alexpott and I discussed that the underlying concern here is that determining a canonical sort order for modules based on dependencies isn't the responsibility of the ConfigImporter -- it belongs to the extension system and should be the same everywhere. That's why this fragility cropped up (aside from PHP being silly), that's why the test coverage seems insufficient to me and out of scope to @Berdir, and that's why it seemed wrong for the array sort logic to be inlined that way. So, I'm going to reroll with one inline multisort for uninstallation -- complete with detailed comment explaining the intended sort order -- that is reversed for the install order. Then, we can add a followup to move this functionality to the extension system, which already has internal support for other sorting, as shown in #19.

Here we go. Interdiff is against the original patch in #10. Still need to file the followup.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -370,16 +370,33 @@ protected function createExtensionChangelist() {
   +    // Sort the list of newly uninstalled extensions by their weights, so that
   +    // dependencies are uninstalled last. Extensions of the same weight are
   +    // sorted in reverse alphabetical order, to ensure the order is exactly
   +    // opposite from installation. For example, this module list:
   +    // array(
   +    //   'actions' => 0,
   +    //   'ban' => 0,
   +    //   'options' => -2,
   +    //   'text' => -1,
   +    // );
   +    // will result in the following sort order:
   +    // 0   options
   +    // 1   text
   +    // 2 0 ban
   +    // 2 1 actions
   +    // @todo Move this sorting functionality to the extension system.
   +    array_multisort(array_values($module_list), SORT_ASC, array_keys($module_list), SORT_DESC, $module_list);
   

   Not sure I understand...

   The input example contains no modules with a weight of 1 or 2, but yet, the comment states those weights in the first sort order...? -1 and -2 do not appear?

   The second dimension + example values seem to be correct though.

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -370,16 +370,33 @@ protected function createExtensionChangelist() {
   +    // (with dependencies installed first, and modules of the same name sorted
   +    // in alphabetical order).
   

   "...of the same weight sorted..."

Fixes sun's comments.

Thanks!

I think we can move forward here — even though my last question was why we're first sorting reversely and later reverse it again. At least the multisort is harder to understand, as it's very uncommon that you see a module extension list being sorted completely "upside-down".

Yeah the sorting in reverse order, then reversing it to get the opposite is a bit odd, but I think we can sort that when we move it to the extension system. Committed/pushed to 8.x, thanks!