Filter module: Reset text format editor settings if editor is changed

[indigoxela]

Description of the bug

Currently we have CKEditor 4, which is possibly the only editor with settings via the filter module admin UI (could be wrong), which also uses a setting named "toolbar".

If switching the editor, for instance on /admin/config/content/formats/filtered_html, the callback fires to load the new editor's settings. However, the $format variable of the form (filter_admin_format_form) isn't updated, which leads to all sorts of errors because of the wrong data structure.

Steps To Reproduce

Yeah, that's the tricky part. Install the new CKEditor5 module from contrib, and try to get along (patches) with all its quirks. Just to figure out, that the filter module sticks with outdated and incompatible settings in the editor settings form.

Edit: take the mini-module "dummyeditor" from below comment for local testing.

The PHP version might also play a role, didn't test below PHP 8.1.

Actual behavior

All sorts of PHP errors and warnings, depending in which direction one tries to switch (4 to 5, or 5 to 4).

Expected behavior

Smooth switching. 😉

Additional information

This is a blocker for progress on the new CKEditor5 module, which is a candidate to get merged to core with our 1.27 release next January.

[indigoxela]

Here's a dummy module to test the problem locally:

<?php

/**
 * Implements hook_editor_info().
 */
function dummyeditor_editor_info() {
  $editors['dummyeditor'] = array(
    'label' => t('Dummy Editor'),
    'settings callback' => '_dummyeditor_settings',
    'default settings' => array(
      // The setting name "toolbar" clashes with CKEditor 4.
      'toolbar' => array(
        'bold' => 0,
        'italic' => 0,
        'underline' => 0,
      ),
    ),
  );
  return $editors;
}

/**
 * Custom callback.
 */
function _dummyeditor_settings(&$form, $form_state, $format) {
  $elements = array();
  $settings = $format->editor_settings;
  $elements['toolbar'] = array(
    '#title' => t('Dummy setting'),
    '#type' => 'checkboxes',
    '#options' => array(
      'bold' => t('Bold'),
      'italic' => t('Italic'),
      'underline' => t('Underline'),
     ),
    '#default_value' => $settings['toolbar'],
  );
  return $elements;
}

When opening a format, that has the Dummy Editor selected (saved previously), then switching to CKEditor, lots of errors should show up on next page call (ajax triggered errors don't show up immediately, but on next page call). Note that after switching, the CKEditor toolbar is empty, although the defaults should have been loaded.

That the Filter module doesn't clean up previous settings, only shows up as errors, if setting names clash, otherwise settings just all get stuffed into the $format object and the editors pick what's they need.

[indigoxela]

Time for a rebase, done. 😉 (Nice, that Tugboat works again, meh, my favourite random failure again.)

Although we've been able to workaround this bug, the issue still seems valid to me.

[quicksketch]

I thought that might be the problem. So the settings from one editor get applied to the other? That would explain why settings like file and image upload persist between CKE4 and 5. It's sorta convenient, but at the same time shouldn't be happening because those settings could be entirely different between the two editor implementations (like "toolbar" is).

[quicksketch]

@indigoxela I pushed a commit to the PR to fix a potential issue. hook_editor_info() does not require default settings to be specified. While unlikely, it's possible. I added default settings to default to an empty array for all editors in the event that they don't provide one in hook_editor_info(). Since filters have the same problem, I also provided a default there. This makes it safe to check $editor['default settings'] and $filter['default settings'] without isset() statements everywhere.

[indigoxela]

I pushed a commit to the PR to fix a potential issue. hook_editor_info() does not require default settings to be specified

Many thanks for fixing, makes sense. 👍

[quicksketch]

I merged backdrop/backdrop#4503 into 1.x and 1.26.x. Thanks @indigoxela!