Issue #3465033: Fix not saving addItemToToolbar recipe action for divider | (vertical separator) or wrapping - items to CKEditor 5

Rajab Natshahrequested to merge
issue/drupal-3465033:3465033-11-x into main

Issue: https://www.drupal.org/project/drupal/issues/3465033

Problem

AddItemToToolbar::apply() returns early when the item being added is a divider |, before the $editor->setSettings($editor_settings)->save() at the end of the method. The recipe action reports success and persists nothing, so a | added by a recipe never reaches the text format's stored settings.

Separately, the duplicate guard exempts only |. A toolbar may legitimately carry more than one wrapping separator -, but a second - added by a later recipe is silently dropped.

What actually still reproduces on main

Running the new test coverage against unpatched current main gives 5 failures out of 16, and that narrows the issue as originally reported:

  • | is genuinely not saved. The early return skips the save() at the end of apply(). 3 cases fail. This is the bug as filed.
  • - is already saved on current main. It falls past the | check, passes harmlessly through the plugin-definition loop (no plugin declares - as a toolbar item) and reaches the save() at the end of the method. Those 3 cases pass unpatched. The "wrapping -" half of the issue title, as a not saved bug, no longer reproduces.
  • What is still broken for - is the duplicate guard. Only | was exempt, so a second - is silently dropped. Both testSeparatorsAreNeverDuplicates cases fail unpatched. This is a different defect from the one filed.

Changes

core/modules/ckeditor5/src/Plugin/ConfigAction/AddItemToToolbar.php

(a) Save before the separator early return, and cover - alongside |, so both take the same cheap exit:

// If we're just adding a divider (vertical separator) or a wrapping
// separator, there's nothing else we need to do at this point.
if ($item_name === '|' || $item_name === '-') {
  $editor->setSettings($editor_settings)->save();
  return;
}

(b) Exempt - from the duplicate guard as well as |:

if (in_array($item_name, $editor_settings['toolbar']['items'], TRUE) && $allow_duplicate === FALSE && $item_name !== '|' && $item_name !== '-') {

with the comment extended to say that dividers and wrapping separators are exempt because a toolbar can legitimately contain more than one of each.

Change (b) is one line beyond this issue's original scope. It is here because the tests written against the issue title expose it. If a reviewer would rather it were split into its own issue, say so and it will be moved out.

core/modules/ckeditor5/tests/src/Kernel/ConfigAction/AddItemToToolbarConfigActionTest.php (+41)

  • 6 new #[TestWith] cases on testAddItemToToolbar covering | and -, each as a bare string, as ['item_name' => ...], and with a position. They assert against a freshly loaded Editor entity, so an unsaved change fails rather than passing on in-memory state.
  • New testSeparatorsAreNeverDuplicates(string $item_name), #[TestWith(["|"])] and #[TestWith(["-"])], applying two separate recipes that each add the same separator.

Net diff against main: 2 files, +48 / -6.

Branch state

Rebased on 14 September 2026 from the original 31 July 2024 base onto main at 55148447cf, and force-pushed. New head d1de382aac. The pre-rebase head 28777fda74 is preserved on the issue fork as tag previous/3465033-11-x/2026-09-14.

This answers the rebase request in #16.

Verification

Locally, on a DDEV site built from the issue-fork checkout at the rebased head, PHP 8.5.9 (core main requires >= 8.5.0), MySQL:

  • With the fix: OK (16 tests, 38 assertions) for core/modules/ckeditor5/tests/src/Kernel/ConfigAction.
  • With AddItemToToolbar.php reverted to main: Tests: 16, Failures: 5 — the 3 | cases plus both testSeparatorsAreNeverDuplicates cases.
  • phpcs --standard=core/phpcs.xml.dist on both changed files: clean.
  • phpstan analyse -c core/phpstan.neon.dist on both changed files: [OK] No errors.

CI: pipeline 959980 on d1de382aac passed — 22 jobs green, including PHPUnit Kernel 4/4, Functional 8/8, Functional JavaScript 3/3, PHPUnit Build, Nightwatch and PHPCS.

Two jobs failed, both allow_failure: true, and neither is related to this change:

  • 📋 PHPUnit Unit (Component): [8.6-ubuntu]
  • 📋 PHPUnit Unit (Core): [8.6-ubuntu]

Both are mikey179/vfsstream calling spl_object_hash(), deprecated in PHP 8.6, surfacing as InfoParserException: Unable to parse vfs://drupal/…/module_a.info.yml in UpdateRegistryTest and friends. Every ckeditor5 unit test in that job passed. The PHP 8.5 counterparts of both jobs are green.

Known interaction

Per #16, this may eventually be affected by #3507570 (allow a recipe to add multiple buttons to the CKEditor 5 toolbar). Noted as an interaction to watch, not a blocker.

Sibling merge requests

!9004 (closed) (10.4.x) and !9003 (closed) (10.3.x) are not rebased and do not carry this test coverage yet.

AI policy

AI-Generated: Yes — produced with Claude Code. Rajab Natshah authored the change and is accountable for it. It has had no independent review yet, which is why Reviewed by human below is unticked.

Checkpoints:

  • File an issue
  • Addition/Change/Update/Fix
  • Testing to ensure no regression
  • Automated unit testing coverage
  • Automated functional testing coverage
  • UX/UI designer responsibilities
  • Readability
  • Accessibility
  • Performance
  • Security
  • Developer Documentation
  • User Guide Documentation
  • Reviewed by human
  • Code review by maintainers
  • Full testing and approval
  • Credit contributors
  • Review with the product owner
  • Release notes snippet
  • Release
Edited by Rajab Natshah

Merge request reports