Problem/Motivation
Sporadically we get the following warning:
Warning: uasort(): Array was modified by the user comparison function in Drupal\Core\Plugin\DefaultLazyPluginCollection->sort() (line 100 of core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php).
A 100% repo is to use the "Entity Browser Module" when using a file field (in our case inside a field collection) with the File Browser widget.
Proposed resolution
According to this link:
http://shout.setfive.com/2013/06/14/symfony2-usort-array-was-modified-by...
The problem lies in the lazy loading of the plugin collection. So the fix is simple. In the sort() function (DefaultLazyLoadingPluginCollection) iterate through the array like this:
public function sort() {
// iterate to populatae the array
foreach($this->instanceIDs as $id){
$pluginId = $this->get($id)->getPluginId();
}
// original code
uasort($this->instanceIDs, array($this, 'sortHelper'));
return $this;
}
Remaining tasks
reviews needed, tests to be written and run
User interface changes
none
API changes
none
Data model changes
none
Original report by [username]
none
Comments
Comment #2
joachim CreditAttribution: joachim at Torchbox commentedSeeing this too. For me it happened when I enabled Web Profiler.
Changing to Needs Work, as there's no patch here to review.
Comment #3
niccottrell CreditAttribution: niccottrell commentedI'm getting something similar via a new view:
Comment #4
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedComment #5
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedComment #6
lhuria94 CreditAttribution: lhuria94 commentedComment #7
lhuria94 CreditAttribution: lhuria94 at Srijan | A Material+ Company commentedIterating the IDs through the array. Attaching patch. Please review.
Comment #8
lhuria94 CreditAttribution: lhuria94 at Srijan | A Material+ Company commentedComment #9
lhuria94 CreditAttribution: lhuria94 at Srijan | A Material+ Company commentedComment #10
XanoCan you provide a test case to reproduce this problem, or step-by-step instructions so we can reproduce the problem manually? We would also need to know which versions of PHP cause problems and which ones don't.
Comment #11
RobLoachIn this patch,
$this->get($id)->getPluginId()
is iterated across, but it doesn't actually do anything with `$pluginId`. What does this actually accomplish?Comment #12
Denis Danielyan CreditAttribution: Denis Danielyan as a volunteer commentedThe warning is thrown because the array is modifed while
usort()
is run.According to the supplied link the problem is:
So to fix that
This is done by iterating over
$this->instanceIDs
.Calling
$this->get($id)->getPluginId();
might actually not be needed as lazy loading is triggered by the iterator.Also assigning the result of getPluginId() to a variable is not necessary.
Why is it there then? I don't really remember if the for each loop was optimized away at runtime without the assignment line or if I thought it might happen.
Comment #13
a.milkovskyHaving this issue as well.
I can see the error when I try to add a Media entity(drupal/media_entity_image) to a node.
I can see the same uasort warning but in different classes in other places as well. Modules page /admin/modules and node save with tokens: #2671060: Use SortArray::stableUasort instead uasort to prevent warnings (Array was modified by the user comparison function)
PHP Version: PHP 5.5.9-1ubuntu4.14 (cli) (built: Oct 28 2015 01:34:46)
Drush version: 8.0.2.
#7 hides the warning in this place for me.
This issue is related to the PHP Bug #50688 Using exceptions inside usort() callback function causes a warning.
If you do not have this error, could you please write which PHP version do you use?
Comment #15
Tomefa CreditAttribution: Tomefa commentedIs this patch gonna be include in core on day ?
It correct the functionnality with the media module.
Comment #16
Tomefa CreditAttribution: Tomefa commentedComment #17
geerlingguy CreditAttribution: geerlingguy commentedI've noticed this warning when installing the Lightning module as part of a vanilla BLT configuration, either via a CI tool or locally using
blt local:setup
Comment #18
tenken CreditAttribution: tenken commented@geerlingguy, unless I'm mistaken Lightning is a D8 profile and not a module.
I too just tried installing Lightning via composer and saw the same Warning
Other D8 modules using uasort() report a similar issue also mention this is a known php uasort bug():
https://www.drupal.org/node/2670942 [token module]
php issue:
https://bugs.php.net/bug.php?id=50688 [known php5 issue, fixed in php 7]
I'm not sure what the proper resolution is given it's resolved in some versions of php.
Comment #19
geerlingguy CreditAttribution: geerlingguy commented@tenken - Ah, didn't realize it's been fixed in PHP 7. Hopefully we'll be able to move this particular site I'm working on to PHP 7 soon... then this will be a non-issue :)
Comment #21
phenom CreditAttribution: phenom as a volunteer commentedHello
for a quick fix, add @ before uasort
@uasort($this->instanceIDs, array($this, 'sortHelper'));
related https://www.drupal.org/node/2628884
Comment #22
joachim CreditAttribution: joachim as a volunteer commentedRestoring the version number.
Comment #23
mpotter CreditAttribution: mpotter at Phase2 commentedHere is an update to this patch.
* Removed the un-needed variable assignment
* Improved the comment
* Re-rolled for 8.3.x
Comment #24
shaisamuel CreditAttribution: shaisamuel commented#23 Works for me to fix this error, when image was loading:
uasort(): Array was modified by the user comparison function in /home/devbwfu/public_html/core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php on line 90
Comment #25
iSoLate CreditAttribution: iSoLate commentedPatch #23 fixed it for me.
Comment #29
jhedstromThis fix looks good. It will need test coverage though.
Comment #30
apadernoI guess it needs a re-roll again.
Comment #31
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedPatch #23 re-rolled for 8.9.x-dev.
Comment #32
apadernoIt still needs test coverage, which the patch in the previous comment doesn't include.
Comment #33
jungleOne question, should change $instanceIDs to $instanceIds? could not find where it is documented, I remember, for abbreviation in class methods and properties, it should uppercase the first letter only.
Comment #34
apadernoYes, it should be
LazyPluginCollection::$instanceIds
(see Object-oriented code, Naming conventions), but that can be fixed in a different issue.Comment #35
jungleThanks, @kiamlaluno! A child issue added.
Comment #37
mpotter CreditAttribution: mpotter at Phase2 commentedReroll against latest 8.9.x (works for 8.9.0)
Comment #38
mpotter CreditAttribution: mpotter at Phase2 commentedRan into a new problem with this patch on D8.9.0. When creating a simple Functional test that inherits from `BrowserTestBase` if you tell it to install the `node` module, then even with an empty test I get the error:
Notice this error happens on line 92, which is the new line added by this patch:
Not sure what could cause this since every instance of DefaultLazyPluginCollection should have an instanceIDs property. I'd have to look at what changed in core to understand this more. So just beware when using this patch in 8.9.x
Comment #39
epicflux CreditAttribution: epicflux at Phase2 commented@mpotter it looks like there was a change in the variable name to $this->instanceIds. I updated your patch with that change.
Comment #43
mashot7Is there a patch for 9.3.7?
#39 is not applying.
Comment #44
yogeshmpawarResolved CSpell errors & added interdiff.
Comment #47
quietone CreditAttribution: quietone at PreviousNext commentedA related issue was a bugsmash target this week. Both lendude and I discussed it.
We concluded that this can be closed as outdated. The full details are in #2831964-19: Warning: uasort(): Array was modified by the user comparison function in Drupal\Core\Config\Entity\ConfigEntityListBuilder->load().