Problem/Motivation
@larowlan reported a crash in #3222797-52: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style, which I could not reproduce by just creating a simple test case. It took many hours of trial and error and debugging.
Eventually, the root cause became clear: only if somewhere along the way while various things are computed and data is transformed, certain arrays have an array state where the "next" array key by appending to the array (so $array[] = 'something';) would not be zero (i.e. $array[0] once existed and was since deleted), then \Drupal\ckeditor5\HTMLRestrictions::merge() computes an invalid result.
But … the root cause is not the logic in HtmlRestrictions::merge(). It's in PHP's own array_merge_recursive()! https://php.net/manual/en/function.array-merge-recursive.php does not document this behavior. The consequence is that two ostensibly identical arrays can yield different merge results! 🤯
$a = ['foo' => ['bar' => TRUE]];
$b = ['foo' => 'baz'];
$m1 = array_merge_recursive($a, $b);
$a['foo'][] = 'proof is in the pudding';
unset($a['foo'][0]);
$m2 = array_merge_recursive($a, $b);
var_dump($m1 == $m2);
👆 This prints bool(false)! 🤯🤯🤯
Steps to reproduce
See https://3v4l.org/fXXsn, where I came up with a way to demonstrate the problem and show that there's only once possible solution:
<?php
$a = ['p' => ['id' => TRUE]];
$b = ['p' => FALSE];
print "Behaves as expected.\n";
var_dump(array_merge_recursive($a, $b));
print "\n\nOMG internal array pointer shenanigans can affect the results of array_merge_recursive()!\n";
$a['p'][0] = 'SOMETHING, anything!';
unset($a['p'][0]);
var_dump(array_merge_recursive($a, $b));
print "\n\nThis keeps happening…\n";
var_dump(array_merge_recursive($a, $b));
print "\n\n… not even resetting the internal array pointer helps …\n";
reset($a['p']);
var_dump(array_merge_recursive($a, $b));
print "\n\n… nor casting to object, cloning, then back to array …\n";
$a['p'] = (array) clone (object) $a['p'];
var_dump(array_merge_recursive($a, $b));
print "\n\n… only forcefully recreating a copy-by-value of the array by using array_slice().\n";
$a['p'] = array_slice($a['p'], 0);
var_dump(array_merge_recursive($a, $b));
outputs:
Behaves as expected.
array(1) {
["p"]=>
array(2) {
["id"]=>
bool(true)
[0]=>
bool(false)
}
}
OMG internal array pointer shenanigans can affect the results of array_merge_recursive()!
array(1) {
["p"]=>
array(2) {
["id"]=>
bool(true)
[1]=>
bool(false)
}
}
This keeps happening…
array(1) {
["p"]=>
array(2) {
["id"]=>
bool(true)
[1]=>
bool(false)
}
}
… not even resetting the internal array pointer helps …
array(1) {
["p"]=>
array(2) {
["id"]=>
bool(true)
[1]=>
bool(false)
}
}
… nor casting to object, cloning, then back to array …
array(1) {
["p"]=>
array(2) {
["id"]=>
bool(true)
[1]=>
bool(false)
}
}
… only forcefully recreating a copy-by-value of the array by using array_slice().
array(1) {
["p"]=>
array(2) {
["id"]=>
bool(true)
[0]=>
bool(false)
}
}Proposed resolution
Work around this weakness in array_merge_recursive() by ensuring that the "next" array key of every nested array is always zero.
Remaining tasks
- Tests
- Fix
- Reviews
User interface changes
None.=
API changes
None.
Data model changes
None.
Release notes snippet
None.
Issue fork drupal-3279470
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3279470-htmlrestrictionsmerge-can-crash
changes, plain diff MR !2244
Comments
Comment #3
larowlanI applied this patch on-top of the Styles patch and now when I switch from CKEditor 4 to CKEditor 5 I get an ajax error
Comment #4
wim leers#3: hm … that's surprising. I cannot reproduce that. Just to make sure: did you do
core/rebuild.phpafter applying the patch?If you are sure, can you please share the
editor.editorandfilter.formatconfig entities that are causing this problem for you? 🙏 Otherwise it's going to be nigh impossible for me to reproduce this.Comment #6
wim leersAFAICT #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers would solve this too, because it's removing the use of
array_merge_recursive(). Postponing on that.Comment #7
wim leersThanks to #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers,
array_merge_recursive()is no longer used!🥳