Issue #3396628 by Wim Leers, smustgrave, lauriii, xjm, catch: Fix <ol start> →...

Issue #3396628 by Wim Leers, smustgrave, lauriii, xjm, catch: Fix <ol start> →  native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by

core/modules/ckeditor5/ckeditor5.module

@@ -663,6 +663,40 @@ function ckeditor5_editor_presave(EditorInterface $editor) {
       ];
     }
 
+    // @see ckeditor5_post_update_list_start_reversed()
+    if (in_array('numberedList', $settings['toolbar']['items'], TRUE) && array_key_exists('ckeditor5_sourceEditing', $settings['plugins'])) {
+      $source_edited = HTMLRestrictions::fromString(implode(' ', $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']));
+      $format_restrictions = HTMLRestrictions::fromTextFormat($editor->getFilterFormat());
+
+      // If <ol start> is not allowed through Source Editing (the only way it
+      // could possibly be supported until now), and it is not an unrestricted
+      // text format (such as "Full HTML"), then set the new "startIndex"
+      // setting for the List plugin to false.
+      // Except … that this update path was added too late, and many sites have
+      // in the meantime edited their text editor configuration through the UI,
+      // in which case they may already have set it. If that is the case: do not
+      // override it.
+      $ol_start = HTMLRestrictions::fromString('<ol start>');
+      if (!array_key_exists('ckeditor5_list', $settings['plugins']) || !array_key_exists('startIndex', $settings['plugins']['ckeditor5_list']['properties'])) {
+        $settings['plugins']['ckeditor5_list']['properties']['startIndex'] = $ol_start->diff($source_edited)
+          ->allowsNothing() || $format_restrictions->isUnrestricted();
+      }
+      // Same for <ol reversed> and "reversed".
+      $ol_reversed = HTMLRestrictions::fromString('<ol reversed>');
+      if (!array_key_exists('ckeditor5_list', $settings['plugins']) || !array_key_exists('reversed', $settings['plugins']['ckeditor5_list']['properties'])) {
+        $settings['plugins']['ckeditor5_list']['properties']['reversed'] = $ol_reversed->diff($source_edited)
+          ->allowsNothing() || $format_restrictions->isUnrestricted();
+      }
+      // Match the sort order in ListPlugin::defaultConfiguration().
+      ksort($settings['plugins']['ckeditor5_list']['properties']);
+
+      // Update the Source Editing configuration too.
+      $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags'] = $source_edited
+        ->diff($ol_start)
+        ->diff($ol_reversed)
+        ->toCKEditor5ElementsArray();
+    }
+
     $editor->setSettings($settings);
   }
 }

core/modules/ckeditor5/ckeditor5.post_update.php

@@ -119,7 +119,26 @@ function ckeditor5_post_update_list_multiblock(&$sandbox = []) {
       return FALSE;
     }
     $settings = $editor->getSettings();
+
     // @see ckeditor5_editor_presave()
     return array_key_exists('ckeditor5_list', $settings['plugins']);
   });
 }
+
+/**
+ * Updates Text Editors using CKEditor 5 to native List "start" functionality.
+ */
+function ckeditor5_post_update_list_start_reversed(&$sandbox = []) {
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+  $config_entity_updater->update($sandbox, 'editor', function (Editor $editor): bool {
+    // Only try to update editors using CKEditor 5.
+    if ($editor->getEditor() !== 'ckeditor5') {
+      return FALSE;
+    }
+    $settings = $editor->getSettings();
+
+    // @see ckeditor5_editor_presave()
+    return in_array('numberedList', $settings['toolbar']['items'], TRUE)
+      && array_key_exists('ckeditor5_sourceEditing', $settings['plugins']);
+  });
+}

core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingRedundantTagsConstraint.php

@@ -25,6 +25,13 @@ class SourceEditingRedundantTagsConstraint extends Constraint {
    */
   public $enabledPluginsMessage = 'The following @element_type(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: %overlapping_tags.';
 
+  /**
+   * When a Source Editing element is added that an enabled plugin optionally supports.
+   *
+   * @var string
+   */
+  public $enabledPluginsOptionalMessage = 'The following @element_type(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: %overlapping_tags.';
+
   /**
    * When a Source Editing element is added that a disabled plugin supports.
    *

core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingRedundantTagsConstraintValidator.php

@@ -45,6 +45,8 @@ public function validate($value, Constraint $constraint) {
 
     // An array of tags enabled by every plugin other than Source Editing.
     $enabled_plugin_elements = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($other_enabled_plugins), $text_editor, FALSE));
+    $enabled_plugin_elements_optional = (new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($other_enabled_plugins))))
+      ->diff($enabled_plugin_elements);
     $disabled_plugin_elements = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($enableable_disabled_plugins), $text_editor, FALSE));
     $enabled_plugin_plain_tags = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($other_enabled_plugins), $text_editor, FALSE, TRUE));
     $disabled_plugin_plain_tags = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($enableable_disabled_plugins), $text_editor, FALSE, TRUE));
@@ -65,6 +67,7 @@ public function validate($value, Constraint $constraint) {
     }
 
     $enabled_plugin_overlap = $enabled_plugin_elements->intersect($source_enabled_element);
+    $enabled_plugin_optional_overlap = $enabled_plugin_elements_optional->intersect($source_enabled_element);
     $disabled_plugin_overlap = $disabled_plugin_elements
       // Merge the enabled plugins' elements, to allow wildcards to be resolved.
       ->merge($enabled_plugin_elements)
@@ -73,34 +76,39 @@ public function validate($value, Constraint $constraint) {
       // Exclude the enabled plugin tags from the overlap; we merged these
       // previously to be able to resolve wildcards.
       ->diff($enabled_plugin_overlap);
-    foreach ([$enabled_plugin_overlap, $disabled_plugin_overlap] as $overlap) {
-      $checking_enabled = $overlap === $enabled_plugin_overlap;
+    foreach ([$enabled_plugin_overlap, $enabled_plugin_optional_overlap, $disabled_plugin_overlap] as $overlap) {
+      $checking_enabled = $overlap === $enabled_plugin_overlap || $overlap === $enabled_plugin_optional_overlap;
       if (!$overlap->allowsNothing()) {
         $plugins_to_check_against = $checking_enabled ? $other_enabled_plugins : $enableable_disabled_plugins;
         $plain_tags_to_check_against = $checking_enabled ? $enabled_plugin_plain_tags : $disabled_plugin_plain_tags;
         $tags_plugin_report = $this->pluginsSupplyingTagsMessage($overlap, $plugins_to_check_against, $enabled_plugin_elements);
-        $message = $checking_enabled ? $constraint->enabledPluginsMessage : $constraint->availablePluginsMessage;
+        $message = match($overlap) {
+          $enabled_plugin_overlap => $constraint->enabledPluginsMessage,
+          $enabled_plugin_optional_overlap => $constraint->enabledPluginsOptionalMessage,
+          $disabled_plugin_overlap => $constraint->availablePluginsMessage,
+        };
 
         // Determine which element type is relevant for the violation message.
         assert(count($overlap->getAllowedElements(FALSE)) === 1);
         $overlap_tag = array_keys($overlap->getAllowedElements(FALSE))[0];
         $is_attr_overlap = self::tagHasAttributeRestrictions($overlap, $overlap_tag);
 
-        // If the entirety (so not just the tag but also the attributes, and not
-        // just some of the attribute values, but all of them) of the HTML
-        // elements being configured to be edited via the Source Editing plugin
-        // is supported by a CKEditor 5 plugin, complain. But if some attribute
-        // or some attribute value is still not yet supported, do not generate a
-        // violation message.
-        // If there is overlap, but some attribute/attribute value is still not
-        // supported, exit this iteration without generating a violation
-        // message. Essentially: when assessing a particular value
-        // (for example `<foo bar baz>`), only CKEditor 5 plugins providing an
-        // exact match (`<foo bar baz>`) or a superset (`<foo bar baz qux>`) can
-        // trigger a violation, not subsets (`<foo bar>`).
-        if ($is_attr_overlap && !$source_enabled_element->diff($overlap)->allowsNothing()) {
+        // If one or more attributes (and all of the allowed attribute values)
+        // of the HTML elements being configured to be edited via the Source
+        // Editing plugin is supported by a CKEditor 5 plugin, complain. But if
+        // an attribute overlap is detected due to a wildcard attribute, then do
+        // not generate a violation message.
+        // For example:
+        // - value `<ol start foo>` triggers a violation because `<ol start>` is
+        //   supported by the `ckeditor5_list` plugin
+        // - value `<img data-*>` does NOT trigger a violation because only
+        //   concrete `data-`-attributes are supported by the
+        //   `ckeditor5_imageUpload`, `ckeditor5_imageCaption` and
+        //   `ckeditor5_imageAlign` plugins
+        if ($is_attr_overlap && $source_enabled_element->diff($overlap)->getAllowedElements(FALSE) == $source_enabled_element->getAllowedElements(FALSE)) {
           continue;
         }
+
         // If there is overlap, but the plain tag is not supported in the
         // overlap, exit this iteration without generating a violation message.
         // Essentially when assessing a particular value (for example `<span>`),

core/modules/ckeditor5/tests/fixtures/update/ckeditor5-3396628.php

+<?php
+
+/**
+ * @file
+ * Test fixture.
+ */
+
+use Drupal\Core\Database\Database;
+use Drupal\Core\Serialization\Yaml;
+
+$connection = Database::getConnection();
+
+$format_list_ol_start = Yaml::decode(file_get_contents(__DIR__ . '/filter.format.test_format_list_ol_start.yml'));
+$format_list_ol_start_post_3261599 = Yaml::decode(file_get_contents(__DIR__ . '/filter.format.test_format_list_ol_start_post_3261599.yml'));
+$connection->insert('config')
+  ->fields([
+    'collection',
+    'name',
+    'data',
+  ])
+  ->values([
+    'collection' => '',
+    'name' => 'filter.format.test_format_list_ol_start',
+    'data' => serialize($format_list_ol_start),
+  ])
+  ->values([
+    'collection' => '',
+    'name' => 'filter.format.test_format_list_ol_start_post_3261599',
+    'data' => serialize($format_list_ol_start_post_3261599),
+  ])
+  ->execute();
+
+$editor_list_ol_start = Yaml::decode(file_get_contents(__DIR__ . '/editor.editor.test_format_list_ol_start.yml'));
+$editor_list_ol_start_post_3261599 = Yaml::decode(file_get_contents(__DIR__ . '/editor.editor.test_format_list_ol_start_post_3261599.yml'));
+$connection->insert('config')
+  ->fields([
+    'collection',
+    'name',
+    'data',
+  ])
+  ->values([
+    'collection' => '',
+    'name' => 'editor.editor.test_format_list_ol_start',
+    'data' => serialize($editor_list_ol_start),
+  ])
+  ->values([
+    'collection' => '',
+    'name' => 'editor.editor.test_format_list_ol_start_post_3261599',
+    'data' => serialize($editor_list_ol_start_post_3261599),
+  ])
+  ->execute();

core/modules/ckeditor5/tests/fixtures/update/editor.editor.test_format_list_ol_start.yml

+uuid: f962b8c7-4c74-4100-b6de-08e6a65ff43f
+langcode: en
+status: true
+dependencies:
+  config:
+    - filter.format.test_format_list_ol_start
+  module:
+    - ckeditor5
+format: test_format_list_ol_start
+editor: ckeditor5
+settings:
+  toolbar:
+    items:
+      - numberedList
+      - sourceEditing
+  plugins:
+    ckeditor5_sourceEditing:
+      allowed_tags:
+        - '<ol start foo>'
+image_upload: {  }

core/modules/ckeditor5/tests/fixtures/update/editor.editor.test_format_list_ol_start_post_3261599.yml

+# Identical to editor.editor.test_format_list_ol_start_post_3261599, except for the plugin configuration.
+uuid: f962b8c7-4c74-4100-b6de-18e6a65ff43f
+langcode: en
+status: true
+dependencies:
+  config:
+    - filter.format.test_format_list_ol_start_post_3261599
+  module:
+    - ckeditor5
+format: test_format_list_ol_start_post_3261599
+editor: ckeditor5
+settings:
+  toolbar:
+    items:
+      - numberedList
+      - sourceEditing
+  plugins:
+    ckeditor5_list:
+      properties:
+        reversed: false
+        startIndex: true
+      multiBlock: true
+    ckeditor5_sourceEditing:
+      allowed_tags:
+        - '<ol foo>'
+image_upload: {  }

core/modules/ckeditor5/tests/fixtures/update/filter.format.test_format_list_ol_start.yml

+uuid: 343a36d6-5852-45f5-b4de-437551cb9caf
+langcode: en
+status: true
+dependencies: {  }
+name: 'Test format — list — <ol start>'
+format: test_format_list_ol_start
+weight: 0
+filters:
+  filter_html:
+    id: filter_html
+    provider: filter
+    status: true
+    weight: -10
+    settings:
+      allowed_html: '<br> <p> <ol start foo> <li>'
+      filter_html_help: true
+      filter_html_nofollow: false

core/modules/ckeditor5/tests/fixtures/update/filter.format.test_format_list_ol_start_post_3261599.yml

+# Identical to filter.format.test_format_list_ol_start_post_3261599.
+uuid: 343a36d6-5852-45f5-b4de-a37551cb9caf
+langcode: en
+status: true
+dependencies: {  }
+name: 'Test format — list — <ol start> — post-#3261599'
+format: test_format_list_ol_start_post_3261599
+weight: 0
+filters:
+  filter_html:
+    id: filter_html
+    provider: filter
+    status: true
+    weight: -10
+    settings:
+      allowed_html: '<br> <p> <ol start foo> <li>'
+      filter_html_help: true
+      filter_html_nofollow: false

core/modules/ckeditor5/tests/src/Functional/Update/CKEditor5UpdateOlStartReversed.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\ckeditor5\Functional\Update;
+
+use Drupal\ckeditor5\HTMLRestrictions;
+use Drupal\editor\Entity\Editor;
+use Drupal\FunctionalTests\Update\UpdatePathTestBase;
+
+/**
+ * @covers ckeditor5_post_update_list_start_reversed
+ * @group Update
+ * @group ckeditor5
+ */
+class CKEditor5UpdateOlStartReversed extends UpdatePathTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setDatabaseDumpFiles() {
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-9.4.0.filled.standard.php.gz',
+      __DIR__ . '/../../../fixtures/update/ckeditor5-3396628.php',
+    ];
+  }
+
+  /**
+   * Test that sites with <ol start> or <ol reversed> opt in to the expanded UI.
+   */
+  public function testUpdate(): void {
+    $before = Editor::loadMultiple();
+    $this->assertSame([
+      'basic_html',
+      'full_html',
+      'test_format_list_ol_start',
+      'test_format_list_ol_start_post_3261599',
+      'test_text_format',
+    ], array_keys($before));
+
+    // Basic HTML before: only <ol type> editable via Source Editing … but just
+    // like a real site, this update path was added too late, and many sites
+    // have in the meantime edited their text editor configuration through the
+    // UI, in which case they may already have set it. That is also the case for
+    // the test fixture used by update path tests.
+    $settings = $before['basic_html']->getSettings();
+    $this->assertArrayHasKey('ckeditor5_list', $settings['plugins']);
+    $this->assertSame(['reversed' => FALSE, 'startIndex' => TRUE], $settings['plugins']['ckeditor5_list']);
+    $source_editable = HTMLRestrictions::fromString(implode(' ', $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']));
+    $this->assertSame(['type' => TRUE], $source_editable->getAllowedElements()['ol']);
+
+    // Full HTML before: nothing listed for Source Editing.
+    $settings = $before['full_html']->getSettings();
+    $this->assertArrayHasKey('ckeditor5_list', $settings['plugins']);
+    $this->assertSame([], $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']);
+
+    // test_format_list_ol_start before: <ol start foo> using Source Editing.
+    $settings = $before['test_format_list_ol_start']->getSettings();
+    $this->assertArrayNotHasKey('ckeditor5_list', $settings['plugins']);
+    $this->assertSame(['<ol start foo>'], $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']);
+
+    // test_format_list_ol_start_post_3261599 before: <ol foo> for Source
+    // Editing.
+    $settings = $before['test_format_list_ol_start_post_3261599']->getSettings();
+    $this->assertSame(['reversed' => FALSE, 'startIndex' => TRUE], $settings['plugins']['ckeditor5_list']['properties']);
+    $this->assertSame(['<ol foo>'], $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']);
+
+    // test_text_format before: not using the List plugin.
+    $settings = $before['test_text_format']->getSettings();
+    $this->assertArrayNotHasKey('ckeditor5_list', $settings['plugins']);
+
+    $this->runUpdates();
+
+    $after = Editor::loadMultiple();
+
+    // Basic HTML after: reversed=FALSE, startIndex=FALSE, Source Editing
+    // configuration unchanged.
+    $settings = $after['basic_html']->getSettings();
+    $this->assertSame(['reversed' => FALSE, 'startIndex' => TRUE], $settings['plugins']['ckeditor5_list']['properties']);
+    $source_editable = HTMLRestrictions::fromString(implode(' ', $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']));
+    $this->assertSame(['type' => TRUE], $source_editable->getAllowedElements()['ol']);
+
+    // Full HTML after: reversed=TRUE, startIndex=TRUE, and Source Editing
+    // configuration is unchanged.
+    $settings = $after['full_html']->getSettings();
+    $this->assertNotSame($before['full_html']->getSettings(), $after['full_html']->getSettings());
+    $this->assertSame(['reversed' => TRUE, 'startIndex' => TRUE], $settings['plugins']['ckeditor5_list']['properties']);
+    $this->assertSame([], $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']);
+
+    // test_format_list_ol_start after: reversed=FALSE, startIndex=TRUE, and
+    // Source Editing configuration has been updated to only <ol foo>.
+    // Unlike the basic_html editor, this one was not yet modified by the user
+    // on the site, so it does not yet have `settings.plugins.ckeditor5_list`.
+    // Hence the missing update path is applied.
+    $this->assertNotSame($before['test_format_list_ol_start']->getSettings(), $after['test_format_list_ol_start']->getSettings());
+    $settings = $after['test_format_list_ol_start']->getSettings();
+    $this->assertSame(['reversed' => FALSE, 'startIndex' => TRUE], $settings['plugins']['ckeditor5_list']['properties']);
+    $this->assertSame(['<ol foo>'], $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags']);
+
+    // test_format_list_ol_start_post_3261599 after: no changes, because it was
+    // updated from CKEditor 4 post-#3261599, which made this update a no-op.
+    $this->assertSame($before['test_format_list_ol_start_post_3261599']->getSettings(), $after['test_format_list_ol_start_post_3261599']->getSettings());
+
+    // test_text_format after: no changes.
+    $this->assertSame($before['test_text_format']->getSettings(), $after['test_text_format']->getSettings());
+  }
+
+}

core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php

@@ -578,6 +578,60 @@ public function provider(): array {
       ],
       'violations' => [],
     ];
+    $data['INVALID: SourceEditing plugin configuration: <ol start type> must not be allowed because List can generate <ol reversed start>'] = [
+      'settings' => [
+        'toolbar' => [
+          'items' => [
+            'numberedList',
+            'sourceEditing',
+          ],
+        ],
+        'plugins' => [
+          'ckeditor5_list' => [
+            'properties' => [
+              'reversed' => TRUE,
+              'startIndex' => TRUE,
+            ],
+            'multiBlock' => TRUE,
+          ],
+          'ckeditor5_sourceEditing' => [
+            'allowed_tags' => [
+              '<ol start type>',
+            ],
+          ],
+        ],
+      ],
+      'violations' => [
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.0' => 'The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: <em class="placeholder">List (&lt;ol start&gt;)</em>.',
+      ],
+    ];
+    $data['INVALID: SourceEditing plugin configuration: <ol start type> must not be allowed because List can generate <ol start>'] = [
+      'settings' => [
+        'toolbar' => [
+          'items' => [
+            'numberedList',
+            'sourceEditing',
+          ],
+        ],
+        'plugins' => [
+          'ckeditor5_list' => [
+            'properties' => [
+              'reversed' => FALSE,
+              'startIndex' => FALSE,
+            ],
+            'multiBlock' => TRUE,
+          ],
+          'ckeditor5_sourceEditing' => [
+            'allowed_tags' => [
+              '<ol start type>',
+            ],
+          ],
+        ],
+      ],
+      'violations' => [
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.0' => 'The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: <em class="placeholder">List (&lt;ol start&gt;)</em>.',
+      ],
+    ];
 
     return $data;
   }

core/profiles/demo_umami/config/install/editor.editor.basic_html.yml

@@ -36,7 +36,7 @@ settings:
     ckeditor5_list:
       properties:
         reversed: false
-        startIndex: false
+        startIndex: true
       multiBlock: false
     ckeditor5_sourceEditing:
       allowed_tags:
@@ -47,16 +47,16 @@ settings:
         - '<a hreflang>'
         - '<blockquote cite>'
         - '<ul type>'
-        - '<ol type start>'
+        - '<ol type>'
         - '<h2 id>'
         - '<h3 id>'
         - '<h4 id>'
         - '<h5 id>'
         - '<h6 id>'
         - '<img src alt data-entity-type data-entity-uuid data-align data-caption width height loading>'
-        - '<drupal-media data-view-mode title>'
+        - '<drupal-media title>'
     media_media:
-      allow_view_mode_override: false
+      allow_view_mode_override: true
 image_upload:
   status: false
   scheme: public

core/profiles/demo_umami/config/install/filter.format.basic_html.yml

@@ -63,6 +63,7 @@ filters:
     settings:
       default_view_mode: responsive_3x2
       allowed_view_modes:
+        medium_8_7: medium_8_7
         responsive_3x2: responsive_3x2
       allowed_media_types:
         audio: audio

core/profiles/standard/config/install/editor.editor.basic_html.yml

@@ -49,7 +49,7 @@ settings:
         - '<a hreflang>'
         - '<blockquote cite>'
         - '<ul type>'
-        - '<ol start type>'
+        - '<ol type>'
         - '<h2 id>'
         - '<h3 id>'
         - '<h4 id>'