Bug in CKE 4 → 5 upgrade path "format" does not always map to "heading", it could map to "codeBlock" too, or both, or neither

RTBC, just a very minor readability concern

1. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -214,6 +221,15 @@ public static function emptySet(): HTMLRestrictions {
   +  public function isUnrestricted(): bool {
   

   Seems fine to add this to the API since this is something is useful to be able to check 👍

2. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -249,6 +265,18 @@ public static function fromTextFormat(FilterFormatInterface $text_format): HTMLR
   +    // @todo Refine in https://www.drupal.org/project/drupal/issues/3231336, including adding support for all operations.
   +    $restrictions = HTMLRestrictions::emptySet();
   +    $restrictions->unrestricted = TRUE;
   

   Seems fine since this is all internal. 👍

3. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Core.php
   @@ -115,7 +120,21 @@ public function mapCKEditor4ToolbarButtonToCKEditor5ToolbarItem(string $cke4_but
   +        $intersect = array_intersect(['h2', 'h3', 'h4', 'h5', 'h6'], array_keys($allowed_elements));
   

   Could there be wildcards like <h*>?

4. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Core.php
   --- a/core/modules/ckeditor5/src/Plugin/CKEditor4To5UpgradePluginInterface.php
   +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5UpgradePluginInterface.php
   
   +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5UpgradePluginInterface.php
   @@ -35,7 +42,7 @@ interface CKEditor4To5UpgradePluginInterface extends PluginInspectionInterface {
   +  public function mapCKEditor4ToolbarButtonToCKEditor5ToolbarItem(string $cke4_button, HTMLRestrictions $text_format_html_restrictions = NULL): ?string;
   

   Wondering about the new argument being optional. Since it's required for some plugins, shouldn't we have it as a required argument even though that would be a BC break?

5. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Core.php
   @@ -69,7 +70,11 @@ class Core extends PluginBase implements CKEditor4To5UpgradePluginInterface {
   +      throw new \LogicException('This @CKEditor4To5Upgrade plugin requires the optional $text_format_html_restrictions argument.');
   

   Ahh I see, it is marked as required here 🤔 I guess this is better for BC reasons + this should be only called by \Drupal\ckeditor5\Plugin\CKEditor4To5UpgradePluginManager.

6. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5UpgradePluginInterface.php
   @@ -24,10 +25,16 @@ interface CKEditor4To5UpgradePluginInterface extends PluginInspectionInterface {
   +   *   In rare situations, a single toolbar button in CKEditor 4 is mapped to
   +   *   multiple toolbar items in CKEditor 5. In this case, a string is still
   +   *   returned, but it may contain multiple toolbar items separated by commas.
   

   🥲

wildcards are only for attributes names or values no? using them in tag names triggers a JS error at least on 10.x so not sure it's supposed to work

Looking at the php doesn't seem like <h*> is a valid config.

If we're confident that no-one is implementing the interface yet, I think we should just go ahead and break it (with a release note and CR) before anyone does. ckeditor5 is still experimental and this is required to fix a critical bug.

code-wise it's good with me. I'll let framework managers greenlight it, or not :D

+++ b/core/modules/ckeditor5/src/Plugin/CKEditor4To5UpgradePluginInterface.php
@@ -24,10 +25,16 @@ interface CKEditor4To5UpgradePluginInterface extends PluginInspectionInterface {
    * @return string|null
...
+   *   In rare situations, a single toolbar button in CKEditor 4 is mapped to
+   *   multiple toolbar items in CKEditor 5. In this case, a string is still
+   *   returned, but it may contain multiple toolbar items separated by commas.

Since we are introducing changes to the interface, should we change the return type to an array to properly support this use case?

Still needs a release note. Also tagging for all three branches.

+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -53,6 +53,13 @@ final class HTMLRestrictions {
 
+  /**
+   * Whether this set of restrictions is unrestricted.
+   *
+   * @var bool
+   */
+  private $unrestricted = FALSE;
+

This is confusing with the double negative, could it use isEmpty or isEmptySet or similar instead?

I personally think unrestricted makes the most sense it's a term that's already being used in Editor and CKEditor 5. This is specifically trying to explain why the restriction set is empty - is it because nothing is allowed or because everything is allowed.

I think we need something better than "Whether this set of restrictions is unrestricted." for the comment then.

OK that reads a lot better, I changed 'f.e.' to 'e.g.' on commit.

Committed/pushed to 10.0.x, 9.4.x, and 9.3.x, thanks!

Given that this was backported to 9.3.x for a release note about a technical BC break, I don't think we don't need to mention it in the 10.0 and 9.4 release notes. It's a very small change from 10.0.0-alpha2 compared to the many significant BC breaks from major dependency updates and deprecation removals for things that were not deprecated until this month, and 9.4.x has never had a tagged release.