Starting with the commit added by #2648334: Implement action/condition reordering with drag & drop, we are now seeing a few test failures with PHP 5.5 on Drupal 8.7. PHP 7 on Drupal 8.7 works fine, and the code and tests work fine for every other supported configuration.

For a full explanation of why see #3092087-20: Deprecate 'context' in Rules @RulesAction annotation

My plan at this time is not to fix these, because they are PHP problems that require a lot of effort to work around. But if you are interested in doing the work I would be happy to use it. See the above issue for details.

CommentFileSizeAuthor
#7 3101013-7.mergesort-for-php5.patch4.55 KBjonathan1055

Comments

TR created an issue. See original summary.

tr’s picture

tr commented

Seems like this has been a problem in core for a long time too, but still not solved. It core gets fixed, I would use their new stable sort method and that would be the end of it.
#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
#2759479: Proof of concept: Replace uasort() with dedicated stable-sort-by-weight functions, where possible

jonathan1055’s picture

jonathan1055 commented
Title: PHP 5.5 test fails » PHP 5.5 - conditions and actions may execute in wrong order if all weights are default

Good research. Interetsing to see that Core have already had plenty of discussion on this.

To quote from the issue where this was discovered:

In PHP 5 the execution order may be wrong by default if all the weights are =0, which is the default weight that is set when a condition/action is first added. To work around this in PHP 5 you will have to either manually set the weights in the UI (you can do this via dragging or by showing the weights and entering distinct values in the textfields) OR you can use this mergesort-test.patch from #3092087-21: Deprecate 'context' in Rules @RulesAction annotation

Putting this here, and modifying the title so that others can more easily discover this issue.

tr’s picture

tr commented

OK, good. Now if someone is by chance using PHP 5 still and encounters this problem we can point to this issue and the workaround. Thus they will be able to still use Rules.

jonathan1055’s picture

jonathan1055 commented

You may have already had this idea and discarded it, but just wondering ... when an action or condition is first added, if no weight is specified could we get the highest weight so far added and increment it to use for the new weight, so that we do not have all zeros? I'm sure it wont be as easy as that, so I have probably missed some obvious point.

tr’s picture

tr commented

Yes, I did think of that, but I never pursued it because my thought was that when adding a new condition/action I would have to loop over all the existing conditions/action, get their weights, then set the weight of the new condition/action to be 1 greater than the max. Then what happens if that +1 exceeds the maximum weight? I just really didn't want to have to micromanage the weights when that is something that should be taken care of for me by tabledrag.

It's possible there's an easy way to do this, but I'm really not motivated to try to find one. But it someone does find a simple and clean way to do this then I would be glad to commit it.

jonathan1055’s picture

jonathan1055 commented
Status: Postponed » Needs review
StatusFileSize
new 3101013-7.mergesort-for-php5.patch4.55 KB

I agree we don't want to make it harder for us to maintain the Rules code, and adding new 'micro-managing' of the weight certainly falls into that category, plus the new tests for it would also add to the burden.

However, I do think it is worth catering for PHP5 whilst it is supported by Drupal. Hence I have taken your mergesort patch, and made a simple conditional in the two getIterator functions, testing the PHP version. We can get rid of this code when we no longer need to support PHP. Yes the mergesort function is slower, but then PHP5 is slower anyway. The impact when running PHP7 will be negligable.

We do not need any new tests for this, because as we have seen already, the current tests show that we have failures without it, but will pass with it and the default order is maintained.

  • TR committed 7837688 on 8.x-3.x authored by jonathan1055 
    Issue #3101013 by jonathan1055: PHP 5.5 - conditions and actions may...
tr’s picture

tr commented
Status: Needs review » Fixed

OK, I agree we should do it as a temporary solution. Can you open a new issue for removing this, post the reverse patch, and mark it postponed until D8.8 is the lowest supported version? I don't want to keep this in there any longer than we need to.

jonathan1055’s picture

jonathan1055 commented
Related issues: +#3105325: Remove PHP mergesort function when core 8.8 is the lowest supported version

Thanks for committing, it is good to see the green tests at PHP5

I have raised #3105325: Remove PHP mergesort function when core 8.8 is the lowest supported version as the follow-up.

Status: Fixed » Closed (fixed)

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