Follow-up to #2448765: Element::children sort order undefined and slower than it could be - This makes tests fail in PHP7.
The other issue turned Element::children() into a stable sort (preserving the order in case of equal weight).
For a stable sort we don't need the funky float weights.
I suspect this group sort is better. Simpler for sure, faster maybe.
Lets just see if it passes.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | D8-2522712-19-FAIL-Element-children-stable-sort-3.patch | 3.95 KB | donquixote |
| #19 | D8-2522712-19-Element-children-stable-sort-3.patch | 3.53 KB | donquixote |
Comments
Comment #2
donquixote commentedForgot to set
$elements['#sorted'] = TRUE.Comment #4
donquixote commentedTwo new versions of the patch.
We need to keep the factor 1000 on the weight, otherwise the checkboxes and radios won't work correctly.
A follow-up patch will change checkboxes and radios so they no longer need this.
Comment #5
donquixote commentedChanged the checkboxes and radios, so we can remove the factor 1000 from the weight.
Comment #6
donquixote commentedComment #9
donquixote commentedSlightly different approach.
Instead of multiplying by 1000, we can simply convert to string to preserve float weights.
This means we no longer need to do anything about Checkboxes - although we can simplify it later.
The difference to previous patches is quite big, I won't post an interdiff.
Comment #10
donquixote commentedNo need to remove this comment..
Comment #11
donquixote commentedD8-2522712-11
A few minor changes to the patch from #10.
D8-2522712-11-FAIL
Attempt to optimize, expected to fail.
This should work in theory, but there is a problem with FilterFormatEditForm / FilterFormatFormBase.
With this patch, the vertical tabs don't show up.
Somehow the "detail" elements in the render array are references. The Element::children() rearranging (unset + reinsert) destroys those references. However, the optimization in D8-2522712-11-FAIL does only rearrange some, but not all, of the children. The remaining children remain as references, which causes them to disappear.
This has to be analysed in a separate issue.
Comment #14
donquixote commentedReroll.
Comment #15
donquixote commentedDuh. Wrong direction.
Comment #16
donquixote commentedComment #19
donquixote commentedComment #20
donquixote commentedComment #22
donquixote commentedGreat!
The patch that is meant to work passes the tests. Back to "Needs review".
The other patch is expected to fail, until #2527762: FilterFormatEditForm vertical tabs not working without #weight. is fixed.
Comment #24
fabianx commentedI appreciate the effort. Could you help out to create re-usable helper functions in #2466097: uasort() does NOT preserve sort order, but core assumes so (Element::children, css sort, ...), please?
It is really bad that we need to write the same code again and again ...
Comment #25
donquixote commented@Fabianx: Ok, as a first step, I did post an ignorant comment over there. Not helping, I guess.
Comment #26
donquixote commented@Fabianx: Looking at this again, and also the other issue.
My conclusion for now:
Render arrays are really a special cup of tea. Besides the sorting problem, we need to distinguish element children vs element properties (starting with '#'). And then there is the reference issue described in #11.
I think it is better for performance and for transparency to have a dedicated one-off sort implementation for this specific case. So, exactly what is being proposed in this issue.
This said, I do agree we want reusable sort functions for other cases.
In most of the examples, we are comparing item weights. The items can be arrays or objects, and the weight is e.g.
$item['#weight']or$item->getWeight().So far the approach in the other issue seemed to be to have a stable equivalent to PHP's native
uasort(), with a comparison callback. Poor performance can be expected with this approach.Instead, we should first extract all the weights, sort the keys by weight (stable), and then build the sorted array of items.
There are different ways to implement this. The idea is mostly the same, but there is difference in the detail. We need to find out which one is faster.
In fact we can have more than one function. E.g. one to sort by
$item[$weight_key], another to sort by$item->$weightMethodName(), another to sort by$weight_callback($item). Maybe special variation with a hardcoded weight method name, e.g.$item->getWeight().I personally think this does not belong in Drupal core, but in a 3rd party library.
But even if we do it in Drupal, we should have one dedicated issue + patch + commit which only adds this function with a bunch of tests, and does not fix all the instances where we want to use it. The latter should rather be a separate issue + patch + commit.
The existing issue, #2466097: uasort() does NOT preserve sort order, but core assumes so (Element::children, css sort, ...), is already far too long.
Better start new, I'd say: "Implement a stable-sort-by-weight function".
Comment #38
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Tagging for an issue summary update for what needs to be done in D10
Reading #26 are you saying this and https://www.drupal.org/project/drupal/issues/2466097 should be consolidated?