Ban, Action, Text and Options modules installed in the correct order. Other ConfigImportUITest.php 156 Drupal\config\Tests\ConfigImportUITest->testImport()

Files: 
CommentFileSizeAuthor
#29 2240709.29.patch4.11 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,864 pass(es). View
#29 24-29-interdiff.txt1.26 KBalexpott
#24 interdiff-10-23.patch2.44 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-10-23.patch. Unable to apply patch. See the log in the details link for more information. View
#24 config-2240709-24.patch4.1 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,777 pass(es). View
#14 sort-interdiff.txt3.05 KBxjm
#14 config-2240709-12.patch4.61 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#10 2240709.10.patch3.38 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,777 pass(es). View

Comments

sun’s picture

Issue tags: -Random fail +Random test failure
Berdir’s picture

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...

tim.plunkett’s picture

I get passes and fails locally, on 5.4.4.
The first of two fails in https://qa.drupal.org/pifr/test/769108 is an example of it on the bot.

Berdir’s picture

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.

tim.plunkett’s picture

When I have modules like devel in /modules, I can reproduce. Without them, it passes.

#2240709: ConfigImportUITest::testImport fails when the module list changes tries to add a new test module.

Seems related?

Berdir’s picture

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?

tim.plunkett’s picture

Title: Random fail: ConfigImportUITest::testImport » ConfigImportUITest::testImport fails when the module list changes
Priority: Major » Critical
Issue tags: -Random test failure

This isn't actually random. Locally having non-core modules or trying to rename/add modules in a patch will trigger it 100% of the time.

xjm’s picture

Issue tags: +blocker, +beta target

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

xjm’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
3.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,777 pass(es). View

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...

xjm’s picture

  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.

Berdir’s picture

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.

alexpott’s picture

FileSize
3.53 KB
5.21 KB

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.

xjm’s picture

FileSize
4.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
3.05 KB

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.

xjm’s picture

Assigned: Unassigned » alexpott

lols.

I'll combine these two.

xjm’s picture

Assigned: alexpott » xjm

No this.

Status: Needs review » Needs work

Save a bot for when we need it.

The last submitted patch, 14: config-2240709-12.patch, failed testing.

xjm’s picture

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.

sun’s picture

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...

xjm’s picture

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

xjm’s picture

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

sun’s picture

@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.

xjm’s picture

@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.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
4.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,777 pass(es). View
2.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-10-23.patch. Unable to apply patch. See the log in the details link for more information. View

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

sun’s picture

  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..."

alexpott’s picture

FileSize
1.26 KB
4.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,864 pass(es). View

Fixes sun's comments.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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".

catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • Commit 83f28fa on 8.x by catch:
    Issue #2240709 by xjm, alexpott: ConfigImportUITest::testImport fails...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.