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

  1. Tests
  2. Fix
  3. Reviews

User interface changes

None.=

API changes

None.

Data model changes

None.

Release notes snippet

None.

Issue fork drupal-3279470

Command icon 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:

Comments

Wim Leers created an issue. See original summary.

larowlan’s picture

I 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

Uncaught
Object { message: "\nAn AJAX HTTP error occurred.\nHTTP Result Code: 500\nDebugging information follows.\nPath: /admin/config/content/formats/manage/basic_html?destination=/&ajax_form=1\nStatusText: 500 Service unavailable (with message)\nResponseText: The website encountered an unexpected error. Please try again later.LogicException: The "ckeditor5_style" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "<p class="clear-float"> <h2 class="clear-float"> <h3 class="clear-float"> <h4 class="clear-float"> <h5 class="clear-float"> <h6 class="clear-float"> <a class="button__nav button--arrow-circle-blue"> <table class="js-table--responsive"> <td class="table--cell-width-xll"> <div class="clear-float"> <ul class="listing-links">". in Drupal\\ckeditor5\\Plugin\\CKEditor5PluginManager->getProvidedElements() (line 348 of core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php). Drupal\\ckeditor5\\Plugin\\CKEditor5PluginManager->getEnabledDefinitions() (Line: 648)\nDrupal\\ckeditor5\\SmartDefaultSettings->addToolbarItemsToMatchHtmlElementsInFormat() (Line: 136)\nDrupal\\ckeditor5\\SmartDefaultSettings->computeSmartDefaultSettings() (Line: 266)\nDrupal\\ckeditor5\\Plugin\\Editor\\CKEditor5->buildConfigurationForm() (Line: 177)\neditor_form_filter_format_form_alter() (Line: 562)\nDrupal\\Core\\Extension\\ModuleHandler->alter() (Line: 835)\nDrupal\\Core\\Form\\FormBuilder->prepareForm() (Line: 429)\nDrupal\\Core\\Form\\FormBuilder->rebuildForm() (Line: 628)\nDrupal\\Core\\Form\\FormBuilder->processForm() (Line: 320)\nDrupal\\Core\\Form\\FormBuilder->buildForm() (Line: 73)\nDrupal\\Core\\Controller\\FormController->getContentResult() (Line: 39)\nDrupal\\layout_builder\\Controller\\LayoutBuilderHtmlEntityFormController->getContentResult()\ncall_user_func_array() (Line: 123)\nDrupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->Drupal\\Core\\EventSubscriber\\{closure}() (Line: 564)\nDrupal\\Core\\Render\\Renderer->executeInRenderContext() (Line: 124)\nDrupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)\nDrupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->Drupal\\Core\\EventSubscriber\\{closure}() (Line: 158)\nSymfony\\Component\\HttpKernel\\HttpKernel->handleRaw() (Line: 80)\nSymfony\\Component\\HttpKernel\\HttpKernel->handle() (Line: 58)\nDrupal\\Core\\StackMiddleware\\Session->handle() (Line: 48)\nDrupal\\Core\\StackMiddleware\\KernelPreHandle->handle() (Line: 106)\nDrupal\\page_cache\\StackMiddleware\\PageCache->pass() (Line: 85)\nDrupal\\page_cache\\StackMiddleware\\PageCache->handle() (Line: 48)\nDrupal\\Core\\StackMiddleware\\ReverseProxyMiddleware->handle() (Line: 51)\nDrupal\\Core\\StackMiddleware\\NegotiationMiddleware->handle() (Line: 23)\nStack\\StackedHttpKernel->handle() (Line: 709)\nDrupal\\Core\\DrupalKernel->handle() (Line: 19)\n", name: "AjaxError", stack: "@http://127.0.0.1:8080/sites/default/files/js/js_oqEVgqTRSSiAedNcoXfLEXlf...\n@http://127.0.0.1:8080/sites/default/files/js/js_oqEVgqTRSSiAedNcoXfLEXlf...\n" }
js_oqEVgqTRSSiAedNcoXfLEXlfLhctOmiqn4xjdexIrQU.js:2477:32

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Active » Postponed (maintainer needs more info)

#3: hm … that's surprising. I cannot reproduce that. Just to make sure: did you do core/rebuild.php after applying the patch?

If you are sure, can you please share the editor.editor and filter.format config entities that are causing this problem for you? 🙏 Otherwise it's going to be nigh impossible for me to reproduce this.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

Title: HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state » [PP-1] HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state
Status: Postponed (maintainer needs more info) » Postponed
Related issues: +#3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers

AFAICT #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.

wim leers’s picture

Title: [PP-1] HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state » HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state
Status: Postponed » Closed (outdated)