Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The change to remove each()
functions in \Drupal\Component\Diff\Engine\DiffEngine as part of #2885309: [PHP 7.2] each() function is deprecated has introduced a critical regression that can cause infinite loops when diffing configuration files.
Proposed resolution
Fix logic to be the same as before and add a test.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#9 | 2936941-9.patch | 2.77 KB | alexpott |
#9 | 5-9-interdiff.txt | 1.5 KB | alexpott |
#5 | 2936941-5.patch | 2.77 KB | alexpott |
#5 | 2-5-interdiff.txt | 5.17 KB | alexpott |
#5 | 2936941-each-5-original-code-using-each.patch | 6.49 KB | alexpott |
Comments
Comment #2
alexpottThe
each()
change didn't refactor keeping the same logic. One to do that would have been to make the following change..
Patch attached makes that change.
The test-only patch here will never complete since the new test is just going to infinitely loop.
Comment #3
alexpottAn alternate fix to HEAD for this would be:
The reason the logic changed is that #2885309: [PHP 7.2] each() function is deprecated didn't account for how each works (from http://php.net/manual/en/function.each.php)
So after it sets
$found_empty
to TRUE in needs to process the same $y value with the next bit of logic.I think I prefer using
current()
andnext()
as in #2 because it keeps our code closer to the original Dairiki diff that the code cam from.Comment #5
alexpottOk I've worked out what is wrong in HEAD. We're in yet another loop so checking
if (!isset($found_empty)) {
is just wrong. We need to set the variable outside the loop. Interesting I couldn't get my version withcurrent
andnext()
to produce the same diff as when we usedeach()
so I've made some changes to assert on what's in the diff to ensure we get the same diff as before. I've also changed the files we're diffing to be a small as possible to cause the infinite loop in heed. So there's less noise. I've also include a patch that reverts DiffEngine to the version before we removedeach()
to show that the new test produces the same diff.Comment #8
alexpottLol Drupal\Tests\Component\Serialization\YamlTest look at you testing all the things.
Comment #9
alexpottComment #12
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@alexpott++ ;-)
Comment #13
catchCommitted/pushed to 8.6.x.
Will cherry-pick to 8.5.x once the alpha is tagged.
Comment #16
catchDone.