element_children sort order inconsistent between PHP 5 and PHP 7 [Drupal 7 backport]

Here's a first try.

I'm not sure how to backport the tests though, I haven't found clear correspondences, shall we create a brand new one with something like https://www.drupal.org/node/2448765#comment-9710393?

Targeting for 7.50. I really would love this in.

IIRC a test was possible with:

One item with weight and two items without weight, but reverse sort order.

Yes, lets re-use that test from nlisgo, that one is great!

I don't think this will make 7.50 and that is a problem for PHP 7.

I think it can go in as a bug fix in ~ 4 weeks though.

The problem is that we had to change things in D8 core for it and that is something I don't like. Also we need to be sure to prevent the (reverse) sort order of older PHP versions, but ensure it is consistent with PHP 7.

As the next step, I updated the patch #4 to add a test like one in #2448765: Element::children sort order undefined and slower than it could be - This makes tests fail in PHP7 discussed above.

There are 3 failed tests with the patch #7 and PHP 7. Seeing the test result page https://www.drupal.org/pift-ci-job/393023 , they're unrelated with this patch, I believe.

Triggering a re-test.

The errors, for the record:

195	1	System.System
1	1	System.IPAddressBlockingTestCase
✓		- setUp
testIPAddressValidation

fail: [Other] Line 732 of modules/system/system.test:
IP address found in database.

fail: [Other] Line 733 of modules/system/system.test:
IP address was blocked.

fail: [Other] Line 739 of modules/system/system.test:
"This IP address is already blocked." found

18	2	Statistics.Statistics
1	2	Statistics.StatisticsBlockVisitorsTestCase
✓		- setUp
	✗	
testIPAddressBlocking

fail: [Other] Line 287 of modules/statistics/statistics.test:
IP address found in database

fail: [Other] Line 288 of modules/statistics/statistics.test:
IP address was blocked.

fail: [Other] Line 293 of modules/statistics/statistics.test:
Unblock IP address link displayed

fail: [Browser] Line 296 of modules/statistics/statistics.test:
Link unblock IP address does not exist on http://localhost/checkout/?q=admin/reports/visitors

fail: [Other] Line 297 of modules/statistics/statistics.test:
IP address deletion confirmation found.

fail: [Other] Line 299 of modules/statistics/statistics.test:
Found the Delete button

fail: [Other] Line 299 of modules/statistics/statistics.test:
Found the requested form fields at admin/config/people/ip-blocking/delete/1

fail: [Other] Line 300 of modules/statistics/statistics.test:
IP address deleted.

	✗	
drupalPost

exception: [Warning] Line 2129 of modules/simpletest/drupal_web_test_case.php:
Invalid argument supplied for foreach()

@stefan.r thank you for your assist! I've retriggered the test for PHP 7 again and it seems to have passed.

To be surer, I'd like to try a test-only patch of #7. It must fail.

I misunderstood it...

In this case, the test-only patch may pass the test. What matter here is that a patch fixes the undefined order problem for PHP 5 and 7.

We have the working patch #7 and I move this to NR again.

The patch looks good and works as intended.

I'm uploading a version that fixes a couple grammatical issues, but no real changes so I'm going ahead and marking this RTBC.

My one question would be whether we actually want this to go out in the next regular bugfix release, or save it for a potential Drupal 7.60 instead? By changing the order in which things get sorted in a render array (even though we are fixing/improving it), it's possible this could lead to some unexpected side effects in certain scenarios.

Well PHP7 is here now, so I think the sooner the better.

If I understand correctly this shouldn't change the sort order for PHP5, only fix it for PHP7, as it's only a bug fix it should be eligible for a bug fix release.

@MustangGB:

If I understand correctly this shouldn't change the sort order for PHP5, only fix it for PHP7, as it's only a bug fix it should be eligible for a bug fix release.

Probably we have this problem on PHP 5 rather than PHP 7 now, I think.

Given:

$element_mixed_weight = array(
  'child5' => array('#weight' => 10),
  'child3' => array('#weight' => -10),
  'child1' => array(),
  'child4' => array('#weight' => 10),
  'child2' => array(),
);

PHP 5.6 returns the following result in my environment.

before fix (unstable):

element_children(element_mixed_weight, TRUE);
// => [
//      "child3",
//      "child2",
//      "child1",
//      "child5",
//      "child4",
//    ]

after fix (stable):

element_children(element_mixed_weight, TRUE);
// => [
//      "child3",
//      "child1",
//      "child2",
//      "child5",
//      "child4",
//    ]

As for PHP 7, PHP 7 changed the behavior of uasort() from PHP 5. But I'm not sure if it's stable or not. (Maybe more stable than PHP 5 but I don't think it's oficially stated.)

I hope this helps.

I get similar results as @hgoto - on PHP 5.5 I'm seeing the new test fail if it's run without the rest of the patch applied. So I think the patch actually is changing the sort order for PHP 5.

Oh, yes that definitely should not happen.

I think it uses reverse order as someone mentioned, so we will need to have the test PASS on PHP 5.5 and fail on 7 and not the other way round.

Thanks, @all.

Thank you for your reviews. I may have misunderstood the problem...

Please let us clarify the point again. In the original issue #2448765: Element::children sort order undefined and slower than it could be - This makes tests fail in PHP7, the problem was explained as following:

- Core uses uasort in quite a few places, but uasort does not preserve the sort order

Hence while:

$test = [
  'foo:1' => [ '#markup' => 'foo - first' ],
  'bar:2' => [ '#markup' => 'bar - second' ],
];

works

$test = [
  'foo:1' => [ '#markup' => 'foo - first' ],
  'bar:2' => [ '#markup' => 'bar - second' ],
  'first:0' => [ '#markup' => 'little things', '#weight' => -1000 ],
];

does not work when run via Element::children($test, TRUE);

It returns:

Array
(
    [0] => first:0
    [1] => bar:2
    [2] => foo:1
)

which is wrong.

This affects Drupal 7 as well as 8 and is the reason why explicit weights are needed so often in 7 even though it _should_ work without.

From this, I understood that this problem is for PHP 5 and also related to some PHP 7 tests failure.

The problem is not that? Is the target PHP 7 rather than PHP 5? So, should we keep the unstable sort feature of element_children() (caused by PHP 5 uasort()) in PHP 7...? Thank you in advance.

#18: No you had been spot on.

I think we just need to reverse the sort result of the children with the same weight (or no weight). (because PHP 7 fixed the order to be stable now!)

While in Drupal 8 we can change the behavior, we can't and should not in Drupal 7.

So we first need a test case that passes on PHP 5.5 and fails on PHP 7.

Then we need a function which still passes with the test applied on PHP 5.5 and passes on PHP 7, too.

Is that clearer now? :)

Thanks!

But unless there's a typo in #15, reversing the sort wouldn't keep things the same either, right? The results there suggest that PHP 5 doesn't consistently do one sort or the other - rather it just looks flat-out unstable:

Given:

$element_mixed_weight = array(
  'child5' => array('#weight' => 10),
  'child3' => array('#weight' => -10),
  'child1' => array(),
  'child4' => array('#weight' => 10),
  'child2' => array(),
);

PHP 5.6 returns the following result in my environment.

before fix (unstable):

element_children(element_mixed_weight, TRUE);
// => [
//      "child3",
//      "child2",
//      "child1",
//      "child5",
//      "child4",
//    ]

(Notice that child1 and child2 get their order switched but child5 and child4 don't.)

Either way, I guess it would be good to do a testbot run of the patch here (plus the tests-only patch) on all PHP versions that the testbot supports, to see what the exact situation is...

@Fabianx @David_Rothstein thank you for your kind feedback.

In order to make the current situation clearer, I'd like to test the following case. This render array is longer than the one in the previous test and will provide clearer hints.

// The order of children with same weight should be preserved.
$element_mixed_weight = array(
  'child5' => array('#weight' => 10),
  'child3' => array('#weight' => -10),
  'child1' => array(),
  'child4' => array('#weight' => 10),
  'child2' => array(),
  'child6' => array('#weight' => 10),
  'child9' => array(),
  'child8' => array('#weight' => 10),
  'child7' => array(),
);

$expected = array(
  'child3',
  'child1',
  'child2',
  'child9',
  'child7',
  'child5',
  'child4',
  'child6',
  'child8',
  );
$this->assertEqual($expected, element_children($element_mixed_weight, TRUE), 'Order of elements with the same weight is preserved.');

I'll trigger the auto test with several php versions.

That looks very good to me.

Nice test coverage.

Thanks for all the great D7 work @hgoto!

OK, so I think the conclusion we've now settled on is that the bug is with PHP 5 (not PHP 7), and that the patch changes behavior for sites running PHP 5 only.

That's fine with me, but there was some confusion about that above so let's make sure everyone is on the same page :) And if so, the question I had in #13 still stands - should we hold this off for Drupal 7.60 given that it can change the order of various render arrays on sites running PHP 5? I would lean towards doing that myself.

Not sure this should be considered a critical bug in that case either.

I agree, lets lower the priority.

Just unfortunate that we can't fix this in a BC keeping fashion.

Still seems worth getting this into a Drupal 7.60 release (and holding it for then given the behavior change).

Question:

+      // Support weights with up to three digit precision and conserve the
+      // insertion order.
+      $child_weights[$key] = floor($weight * 1000) + $i / $count;

Do we care about the loss of precision here? It's a real effect. For an array like this:

$array = array(
  'def' => array('#weight' => 1.000001),
  'abc' => array('#weight' => 1),
);

Before the patch element_children($array, TRUE) returns ['abc', 'def'] as expected, but after the patch it incorrectly returns ['def', 'abc'].

It is probably unusual for code out there to use such precise weights, but maybe we want to up the precision a bit to be safe?

It does seem a bit strange to suddenly only support 3 decimal places, perhaps a better approach would be to get #2466097: uasort() does NOT preserve sort order, but core assumes so (Element::children, css sort, ...)/#2834022: drupal_sort_weight sort order undefined and possibly inconsistent between PHP 5 and PHP 7 in first then use drupal_sort_ordered() instead?

Not even sure if that's possible, but certainly a method that doesn't have a precision limit would be better I think.

I guess that means this needs more discussion, unfortunately.

The problem actually occurs when the weights differ by as much as 0.001 exactly, which is notable because that's an increment Drupal actually uses (for example in https://api.drupal.org/api/drupal/includes%21form.inc/function/form_proc...). Still not sure how much of a practical effect this has, especially since in those cases the array elements are probably already in the correct order so this bug wouldn't necessarily be expected to force them out of order, but it's worrisome nonetheless and does seem like the code could do better.

Bumping to 7.61. This didn't make it into 7.60.

If I'm reading this issue correctly (see e.g. #25) it sounds like the incorrect behaviour was associated with PHP 5 whereas PHP 7 behaves correctly.

Running the 7.x branch's DrupalRenderTestCase tests locally on PHP 7.3, they all pass, including testDrupalRenderSorting() which I believe is the relevant one here.

If that's the case, I'm not sure why we'd need to resolve this as part of the efforts to fix PHP 7 support (I'm specifically looking at #3012308: D7: Fully support PHP 7.3 but the same will be the case once we move on to PHP 7.4 support afterwards).

Therefore, perhaps this should no longer be marked as a child of the PHP 7.3 issue; it is one of the only remaining children not at RTBC and it would be helpful to be able to narrow the children down to only the issues we need to fix in order to be able to be able to mark the PHP 7.3 parent as fixed too.

So I'm going to unmark this as a child; if anyone objects strongly, please share your thinking on the matter :)

@mcdruid

I strongly object, we need a meta issue for PHP 7.x related issues, I don't care if it's #3012308: D7: Fully support PHP 7.3 or #2656548: Fully support PHP 7.0 in Drupal 7 or another, but please let us have a home for them, as far as I'm concerned whilst there are still sub-issues PHP 7 isn't fully supported, so I don't know how it can be justified to close meta issues stating as much.

What mcdruid is saying is that this isn't a compatibility issue Drupal has with PHP 7, so it's not one of the issues to fully support PHP 7.3 in Drupal 7. (In other words, this isn't a child issue of #3012308: D7: Fully support PHP 7.3.)

This is also what David_Rothstein said in comment #25.

OK, so I think the conclusion we've now settled on is that the bug is with PHP 5 (not PHP 7), and that the patch changes behavior for sites running PHP 5 only.

If Element::children() is really slower, that is a performance issue, not a compatibility issue.

Items are sorted in one order in PHP5, and a different order in PHP7, how is that not a PHP7 related issue, anyone upgrading from PHP5 to PHP7 will experience this issue, so it is a compatibility issue for PHP7.

Regardless if the original ordering was "wrong" the point is they are different.

And I don't necessarily agree that PHP5 got it wrong and PHP7 got it right, last I looked into it, it was the opposite way around.

I don't speak in terms of performance, only consistency.

yes I would agree with @MustangGB on this.
The patch provides us consistent behavior and has tests to ensure the consistent sorting.
As for the accuracy mentioned by David Rothstein, 0.001 seems pretty accurate to me for sorting. I'd err on the side of pragmatism, this patch makes the behavior more consistent and people already didn't even notice the sort order difference so what difference does it make if so few people are noticing this. There's a lot of sites running php 7.x and I for one didn't even notice the sort difference but the tests prove the consistent sorting with the new patch. I'd rather have consistency with tests than inconsistency and NO tests.

generally I don't like surprises (inconsistency) and this patch provides consistency which I do like. And yes, in that interpretation it is a php 7 compatibility issue.

@joseph.olstad Well actually we have #2834022: drupal_sort_weight sort order undefined and possibly inconsistent between PHP 5 and PHP 7 now, which IIRC should resolve the 0.001 problem.

Thanks for all the feedback.

I definitely don't want anyone to feel disillusioned that work that's been done is being rejected or left behind, or that their (well argued) opinions are being ignored.

This is probably more a discussion for the meta issue(s) but overall what we're currently trying to work towards is having well defined acceptance criteria for the parent issues for the newer PHP releases. That's where @MustangGB's comment comes in:

as far as I'm concerned whilst there are still sub-issues PHP 7 isn't fully supported

That begs the question "what does Supported mean?" (woah, deep!)

A few possible answers:

• Core tests pass.
• Core and (most?) contrib tests pass.
• New sites launching now on this version of PHP work properly. (Some would question whether anyone is launching new sites on D7 - I'm sure there are some examples.)
• Existing sites migrating from older versions of PHP experience no serious regressions.
• Existing sites migrating from older versions of PHP experience absolutely no change in behaviour (other than perhaps performance improvements).

...you get the idea.

Pragmatically we may be nearer the top of that list than the bottom in deciding whether a new version is "Supported", initially at least.

As I said, this probably belongs in one of the meta issues (or in its own meta issue), but I wanted to be really transparent about why my first pass at the parent issue was tidying up, and seeing if there were any issues we could "de-scope" ... and this one seemed a candidate to me.

In terms of the earlier part of @MustangGB's sentence:

please let us have a home for them

I'm certainly not suggesting that we remove PHP version related tags from issues like this. Does that count as a "home"?

The goal is to triage and prioritise and identify what we can do to make progress with these compatibility parent issues, and having lots of different child issues belonging to various projects and in various states (NR, RTBC etc..) certainly doesn't help make it clear what we need to tackle and in what order.

Perhaps the parent / child relationship isn't the ideal way to tackle this. I've started work on what will eventually be a revised IS for the PHP 7.3 parent, trying to organise and prioritise the different issues a bit. Perhaps that's a better approach than lopping off children.

Let me know what you think, and as I think @joseph.olstad has said elsewhere "thanks for caring" :)

Does that count as a "home"?

The goal is to triage...

Sure, please crack on...

I hope you can understand (even if you don't agree with) the reason for the pushback, that being that unless the communities desires and efforts are aligned with the powers that be's desires and efforts then tough luck.

With so few D7 torch bearers left I only wish we can all pull in the same direction.

Anyway, this issue isn't really the place for such discussions, so yeah, I'll shut up now.

Has tests and tests only proof.

Even though php 5 has faded into the sunset or beyond there is some appeal to having code that supports consistent behaviour and as the patch in this thread provides that it merits committing, imho.

Patch from comment #21 re-rolled for drupal 7.73

Here's a test-only patch based on #48 - I'd like to see the results of (just) the new test with PHP 5 vs. PHP 7.

Izmeez, ironically I know of a very large educational institution (a client of mine from time to time) that has over 180 Drupal 7 websites still actively in development and they are still using php 5.3.x provided by Red Hat, they get security updates backported via their extended support from Red Hat.

So yes, this fix provides consistent behavior through php 7 and php 5, it is a good idea.

Ok, so tests seem to confirm that the patch changes the behaviour in PHP 5 to match PHP 7.

This will need a Change Record to explain that.

Anyone able to draft one please?

I'm happy that this is RTBC, but we still need the CR.

@mcdruid, in D8 this was fixed in 2016 and I checked the D8 issue #2448765 , there is no change notice for D8

Maybe we don't need a change notice since D8 didn't have one.

D8 was released December 2015 , this was fixed in D8 in 2016 without a change notice.

#2448765: Element::children sort order undefined and slower than it could be - This makes tests fail in PHP7

The CR would be to explain to D7 sites running on PHP 5 that the ordering of elements may change as a result of this.

When this was fixed for D8, minimum requirements for D8 were php 5.5.x and php 7.0 was supported. no CR was ever posted for D8 despite the ordering difference.

@mcdruid,
I have created a draft change notice here:
https://www.drupal.org/node/3182011

Please check the draft change notice.

Thanks for the draft CR @joseph.olstad

element_children() is used in so many places, I'm not sure where to start with providing some examples of changes that sites may notice as a result of this being committed.

We could link to the API docs which list the calls to this function in core:

https://api.drupal.org/api/drupal/includes%21common.inc/function/element...

I think it would also be good to provide a couple of examples where core uses this and the patch may cause a noticeable change in the UI.

A couple of concrete examples we could provide:

https://api.drupal.org/api/drupal/includes%21form.inc/function/theme_tab...

https://api.drupal.org/api/drupal/modules%21system%21system.module/funct...

+1 from me - it's good to fix that bug

I also updated the draft CR

Thank you everyone that contributed!