Problem/Motivation
A random fail has been detected in Drupal\comment\Tests\CommentFieldsTest::testCommentInstallAfterContentModule()
exception: [Warning] Line 177 of core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php:
uasort(): Array was modified by the user comparison functionuasort(Array, Array) (Line: 177)
Drupal\Core\Config\Entity\ConfigDependencyManager->getDependentEntities('config', 'field.field.node.article.comment') (Line: 256)
Drupal\Core\Config\ConfigManager->findConfigEntityDependents('config', Array, Object) (Line: 265)
Drupal\Core\Config\ConfigManager->findConfigEntityDependentsAsEntities('config', Array, Object) (Line: 298)
Drupal\Core\Config\ConfigManager->getConfigEntitiesToChangeOnDependencyRemoval('config', Array, ) (Line: 590)
Drupal\Core\Config\Entity\ConfigEntityBase::preDelete(Object, Array) (Line: 193)
Drupal\field\Entity\FieldConfig::preDelete(Object, Array) (Line: 357)
Drupal\Core\Entity\EntityStorageBase->delete(Array) (Line: 366)
Drupal\Core\Entity\Entity->delete() (Line: 596)
Drupal\Core\Config\Entity\ConfigEntityBase::preDelete(Object, Array) (Line: 406)
Drupal\field\Entity\FieldStorageConfig::preDelete(Object, Array) (Line: 357)
Drupal\Core\Entity\EntityStorageBase->delete(Array) (Line: 366)
Drupal\Core\Entity\Entity->delete() (Line: 192)
Drupal\comment\Tests\CommentFieldsTest->testCommentInstallAfterContentModule() (Line: 1057)
Drupal\simpletest\TestBase->run(Array) (Line: 723)
simpletest_script_run_one_test('199', 'Drupal\comment\Tests\CommentFieldsTest') (Line: 59)
Have no idea how this happens... I've run the test well over 100 times locally - no fail.
$graph = $this->getGraph();
uasort($graph, array($this, 'sortGraph'));
return array_replace(array_intersect_key($graph, $dependencies), $dependencies);
public function sortGraph(array $a, array $b) {
$weight_cmp = SortArray::sortByKeyInt($a, $b, 'weight') * -1;
if ($weight_cmp === 0) {
return SortArray::sortByKeyString($a, $b, 'component');
}
return $weight_cmp;
}
Proposed resolution
The graph is sorting on the component key added by the Graph object. It was assumed this was equivalent to the config object name - it is not. Therefore the patch adds the 'name' key and value to the graph so it can use it in the sort. Also it removes the array_reverse() since this doesn't have the expected impact on an already sorted list where the sort is on both weight and alphabet.
Remaining tasks
User interface changes
None
API changes
None - the ConfigDependencyManager public methods now actually return things in the expected order. Previously they correctly ordered things according to dependency (which is the vital thing) but sometimes messed up alphabetical sorting (which is a nice to have - just so things are repeatable and not dependent on PHP version).
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#20 | 2757427-20.patch | 9.1 KB | alexpott |
#20 | 16-20-interdiff.txt | 652 bytes | alexpott |
#16 | 2757427-16.patch | 8.46 KB | alexpott |
#16 | 15-16-interdiff.txt | 7.21 KB | alexpott |
#15 | 2757427-15.patch | 1.72 KB | alexpott |
Comments
Comment #2
alexpottHere's the first know fail... https://www.drupal.org/pift-ci-job/348229
Comment #3
alexpottComment #4
XanoI also checked http://www.php.net/ChangeLog-5.php and no sort-related bugs have been fixed in PHP 5.6, except one with
array_multisort()
which is not what this issue is about.In my experience this exception is also thrown when the sort callback triggers an error or throws an exception internally, without any actual array modifications taking place.
Comment #5
alexpottComment #6
XanoSince I cannot seem to reproduce this locally and 3v4l.org says full graph output is too much to process, let's see what this gets us.
Comment #7
alexpottComment #8
alexpottComment #9
alexpottComment #10
alexpottComment #11
alexpottComment #12
alexpottComment #13
alexpottGiven that this fail is not random on 8.1.x with PHP 5.6.6 making this a major bug.
Comment #14
alexpottComment #15
alexpottSo PHP7 means that we need to sort by name too - therefore we need add the real name to the graph so we can sort it.
Comment #16
alexpottAdded unit tests for more consistent sorting.
Comment #17
alexpottDiscussed with @catch and we agreed this is critical because currently it is preventing a successful 8.1.x test on PHP 5.6. Therefore, arguably 8.1.x is in an unreleaseable state. The problem is the bugs fixed by #2754477: Unexpected config entity delete due to dependency calculations are considerable worse than this one - since this is just a warning and the test actually executes successfully.
Comment #18
alexpottComment #19
xjmYep this is critical per https://www.drupal.org/core/issue-priority#critical-bug:
Comment #20
alexpottOkay so yay! The alphabetically sorting is now working :)
Comment #22
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedDoes this need a change record or is this 100% internal?
--
Besides this, this looks great and is RTBC.
Comment #23
alexpottAlthough the user sort function
\Drupal\Core\Config\Entity\ConfigDependencyManager::sortGraph
is public it really shouldn't be. I think we could file a follow-up issue to make it protected in 8.2.x.Comment #24
alexpottComment #25
alexpottThis is not the expected alphabetical order because the StorageComparer has to reverse the dependencies so that fields are deleted before field storages. Basically it's array_reverse is doing what the one in ConfigDependencyManager::sortAll() was doing - but here it is necessary.
Comment #26
alexpottDiscussed with @xjm, @catch, @cottser, and @effulgentsia we agreed that the persistent fail in PHP 5.6 testing is a critical.
Comment #27
catchNice work tracking this down, nasty one.
Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
Comment #28
Xano@catch: it looks like the push failed: https://www.drupal.org/node/3060/commits does not display any commits referencing this issue.
Comment #30
catchIt did push OK, just something up with drupal.org I think - you can see it on cgit.drupal.org:
http://cgit.drupalcode.org/drupal/commit/?id=3eb2def4460c25cff93940a6331...