Alignment being available as separate buttons AND in dropdown is confusing

1. Keep the dropdown, lose the buttons
2. Make the dropdown configurable (similar to headings), where the user can choose what alignment options will be available in the dropdown.

+1

This change will affect the upgrade path too, so added:

3. Modify the upgrade path in \Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::mapCKEditor4ToolbarButtonToCKEditor5ToolbarItem() to instead generate the dropdown, and then also expand ::computeCKEditor5PluginSubsetConfiguration() to generate the appropriate configuration (much like headings).

Getting this done would help #3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets — this is not a blocker, but it'd make things simpler and clearer there.

Coming here from #3273510: CKEditor 5 crash when multiple alignment buttons are activated due to duplicate configuration.

Pushed my current work on the subject : provide a configurable setup for the dropdown alignment button.

Updating issue "Remaining tasks" : it now takes a CKE 4 to 5 upgrade path and some tests coverage.

Note 1: it is highly likely that the API for the methods for upgrading from CKE 4 to 5 since we will need to keep track of already migrating plugins (or buttons). This is new as it is the first use case of many buttons in CKE4 turning to 1 only with a configuration associated in CK5.

Note 2: I am not sure an upgrade path is necessary for already migrated CKE5 text formats. Indeed, it is currently not possible to use CKE5 with multiple alignments as it crashes, so it is sure the former CKE4 configs are lost as upgrade already occurred and new CKE5 do not have multiple alignments or it would fail.

Thanks so much for pushing this forward, @Dom.! Here's some pointers to help you finish this up:

1. The Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest failure is unrelated. Random failure, unfortunately.
2. Same for 1) Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest::testValidationMessage

3. 1) Drupal\Tests\ckeditor5\Kernel\ConfigurablePluginTest::testDefaults
   

   → simple update of the expectations in that test is all that we need.

4. 1) Drupal\Tests\ckeditor5\Kernel\WildcardHtmlSupportTest::testGhsConfiguration
   

   → you'll need to update the Editor::create() call in \Drupal\Tests\ckeditor5\Kernel\WildcardHtmlSupportTest::testGhsConfiguration() to check if in_array('alignment', $additional_toolbar_items, TRUE) and if so, add the default configuration for the alignment plugin

5. For

   1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testEnabledPlugins
   Failed asserting that two arrays are identical.
   

   you will have to update

       // Case 7: GHS is enabled for other text editors if they are using a
       // CKEditor 5 plugin that uses wildcard tags.
       $settings['toolbar']['items'][] = 'alignment:center';
   

   → that needs to become 'alignment', and you'll need to configure the alignment plugin to only allow center — then the rest of the test should become trivial to update.

6. 1) Drupal\Tests\ckeditor5\Kernel\CKEditor4to5UpgradeCompletenessTest::testCKEditor5ConfigurableSubsetPlugins
   OutOfBoundsException: No upgrade path found for the "ckeditor5_alignment" elements subset configuration.
   

   This needs to be fixed first, and then SmartDefaultSettingsTest should be updated.

   This can be fixed by updating \Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::computeCKEditor5PluginSubsetConfiguration(), look at the case 'ckeditor5_heading': code branch for how to do this, we need something similar for ckeditor5_alignment.

   I think the logic can be as simple as "if any tag allows the text-align-left value on the class attribute, enable the left checkbox for the Alignment plugin's settings". Same thing for right, center and justify.

FYI: this is using the enabled_alignments top-level key for its configuration, which means we could add an tags top-level key in #3259367: [upstream] Allow configuring *which* HTML tags can be aligned, when CKEditor 5 adds upstream support for configuring that.

IOW: this issue does not do things in a way that more configuration could not be added anymore in the future, which is very important! 👍

@Dom. I'll start working on this early next week — no pressure on you at all, just letting you know that if you don't get to it, that's completely fine 😊

One test in SmartDefaultSettingsTest should still fail. I have not yet figured why the <p> tag is added.

Somehow I messed up the branch in the middle. Reverted and rebased on 9.4.x to re-apply cleanly. That make the whole ticket verbose to followers, sorry for that, but easier to take back from here for next developer.

During the CKE4 to 5 process, multiple buttons from CKE4 can merge to one button only in CKE5, for instance the four alignment types in CKE4 now converts to one button only in CKE5.
That creates duplicates during the createSettingsFromCKEditor4 process as we will get multiple times the same button as a result of mapCKEditor4ToolbarButtonToCKEditor5ToolbarItem.

There is two approach:

1. dedupe the buttons in createSettingsFromCKEditor4 after they are all converted.
2. Make mapCKEditor4ToolbarButtonToCKEditor5ToolbarItem aware not only on the current CKE4 button being converted, but also the already converted CKE5 buttons and potentially the other CKE4 buttons to be converted.

Approach 2 has many implications:

• it is confusing for DX as we expect DX to return the the converted CKE5 button, not add it to the already converted buttons which as read-only information
• it is overwhelming information as of separation of concerns regarding what is really done here (map a CK4 button to another in CKE5)
• It would sound similar to what we do in process_hook(&$variable) but with a different type of method signature, once more a new approach as of drupal

For this purpose, I preferred to deduplicate afterward in createSettingsFromCKEditor4. The resulting code is not particularly elegant, maybe some as-readable but more elegant version can be found of this.
The purpose is to remove duplicates and empty values. One of each only must be kept, and the groups '|' must be preserved.

Note: I still have the additional <p> tag added in SmartDefaultSettingsTest.php that I don't know how to solve.

--- Expected
+++ Actual
@@ @@
                 10 => '<h4 id>'
                 11 => '<h5 id>'
                 12 => '<h6 id>'
+                13 => '<p class="text-align-center">'
             )
         )
         'ckeditor5_alignment' => Array &6 (

Also in ValidatorTest.php, the SourceEditingRedundantTagsConstraint->availablePluginsMessage still popups, but with no more 'Alignment' in the overlapping_tags which is now empty. I don't suppose that constraint should popup at all I guess.

#15: You discovered a bug outside the scope of this MR 🤓 (In HTMLRestrictions::fromString(), called by \Drupal\ckeditor5\SmartDefaultSettings::computeNetNewElementsForPlugin().) Pushed ef41c4f5ad2bf8539b5de1fdef8429b71f35938a for that.

You have two choices here:

1. Make the tests introduced in ef41c4f5ad2bf8539b5de1fdef8429b71f35938a pass.
2. Revert that commit/those tests and change
   

       elements:
         - <$text-container class="text-align-left">
         - <$text-container class="text-align-center">
         - <$text-container class="text-align-right">
         - <$text-container class="text-align-justify">
   

   to

       elements:
         - <$text-container class="text-align-left text-align-center text-align-right text-align-justify">
   

Fixing the bug I described in #17 is not stable-blocking. This bug is. So I'm making the decision: reverting the test, and fixing it by working around it, i.e. the second choice in #17.

Doing that would require also changing this bit in \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getProvidedElements():

        elseif (isset($editor)) {
          $subset = $this->getPlugin($id, $editor)->getElementsSubset();
          $subset_violations = array_diff($subset, $defined_elements);
          if (!empty($subset_violations)) {
            throw new \LogicException(sprintf('The "%s" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "%s".', $id, implode(' ', $subset_violations)));
          }
          $defined_elements = $subset;
        }

So we have no choice but to fix the bug in HTMLRestrictions::fromString().

1. I can't seem to trigger the "ckeditor5_alignment" case in \Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::computeCKEditor5PluginSubsetConfiguration. While in CKEditor 4, I dragged the justifyLeft button into the active toolbar, then switched to CKEditor5 and I expected that to work. Am I missing something?

2. in #18, I added the change in \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getProvidedElements():. I'm assuming you also mean to fix the HTMLRestrictions::fromString() bug in this MR. What exactly is the bug? It's not exactly clear to me from the test failure.

3. What test coverage is left that needs to be added?

I committed 2db233e0 to make test runs faster. The number of failures remains the same, meaning only CKE5 tests are failing 👍

Will now do the revert I mentioned on the merge request 15 minutes ago. This should reduce the number of failures. It's not making a meaningful change, it's just reverting a step in the wrong direction because of my confusing comment at #18 🙈

Simply doing the revert I mentioned in #21 fixed most failures! The remaining failures look legitimate: the expectations need to be updated. 👍

#20:

1. Odd, that works fine for me. I wonder if that was due to the bug that was fixed in #21? 🤔
2. See #21. We don't need to fix it here anymore, thanks to changes that #3261599: Use CKEditor 5's native <ol start> support (and also support <ol reversed>) introduced! 😄 There's still a bug though. That's why this issue is tagged Needs followup. I'll create the follow-up issue and add the unit test that I added in #17 (which you reverted). 👍
3. None! 😄

Follow-up issue created: #3278636: HTMLRestrictions::fromString() bug: multiple occurrences of same tag results in only last one being respected.

BTW, this will be way better than in CKEditor 4 already — see #2569893: CKEditor alignment buttons can be used even when the align filter is disabled + #2649546: [upstream] Alignment/justify buttons not appearing for CKEditor, <p class="text-align-left text-align-center text-align-right text-align-justify"> not being allowed automatically.

Let's get this in! 😄

Hook adds configuration I want but editor won't load 🤔

Posted comment on MR with pointers. We want an update path test — see UpdatePathTestBase.

Added upgrade path test but it fails with
Drupal\Component\Plugin\Exception\PluginNotFoundException : The "ckeditor5" plugin does not exist. Valid plugin IDs for Drupal\editor\Plugin\EditorManager are: ckeditor

But the update hook works now, tested it manually by doing steps in #27

Still two unused use statements 😅

Did both of those. #36.2 is an interesting remark — the JsonApiUpdatePathTest example I linked to also has this weakness 😅 The only example that does this better in core is ActionConfigTest: it makes it more specific by including the d.o issue number. So matched that example.

git diff 12fb247a3cb14281e849e123493aac3a094c7f09 060ecfe2e681f1cd1536df9c3b6c7614e9b8b5e9 > 3259593-38.patch

The 9.3.x patch is not applying only because of #3269657: [drupalMedia] The CKEditor 4 → 5 upgrade path for the media_embed filter should not forcefully allow the `data-view-mode` attribute on `<drupal-media>` not yet having been committed to 9.3.x.

And … #41: 🙈 — I was not meticulous enough! This time I ran the update path test locally 😅

Alright, genuinely green now!

So, back to RTBC, and a patch to test against all branches :)

Note: not testing #44 against 9.3.x yet because #3269657: [drupalMedia] The CKEditor 4 → 5 upgrade path for the media_embed filter should not forcefully allow the `data-view-mode` attribute on `<drupal-media>` needs to be cherry-picked first.

It's odd that https://www.drupal.org/pift-ci-job/2380478 lists no useful output at all 🤯

And … I checked out 9.3.x HEAD locally. That works fine. I tested using PHP 7.4 though. Queued a PHP 7.4 test run. Seems to confirm that the problem is specific to PHP 7.3.

I think the output is available here: https://dispatcher.drupalci.org/job/drupal_patches/128502/testReport/jun...

😮 I've never seen that UI before. Odd that it's not available on d.o's usual output page… Since Jenkins results disappear after a while, copy/pasting for posterity:

Testing Drupal\Tests\ckeditor5\Functional\Update\CKEditor5UpdateAlignmentTest
E                                                                   1 / 1 (100%)

Time: 5.78 seconds, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\ckeditor5\Functional\Update\CKEditor5UpdateAlignmentTest::testUpdateAlignmentButtons
Erroneous data format for unserializing 'Symfony\Component\Routing\Route'

/var/www/html/core/lib/Drupal/Core/Routing/RouteProvider.php:252
/var/www/html/core/lib/Drupal/Core/Routing/RouteProvider.php:204
/var/www/html/core/lib/Drupal/Core/Routing/UrlGenerator.php:426
/var/www/html/core/lib/Drupal/Core/Routing/UrlGenerator.php:270
/var/www/html/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php:105
/var/www/html/core/lib/Drupal/Core/Url.php:763
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:376
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:325
/var/www/html/core/tests/Drupal/Tests/UpdatePathTestTrait.php:46
/var/www/html/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:270
/var/www/html/core/modules/ckeditor5/tests/src/Functional/Update/CKEditor5UpdateAlignmentTest.php:42
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

→ the pattern described in #3152660: Symfony routing problems in 9.01 + #3145563: Route serialization incompatibilities between PHP 7.4 and 7.3 (9.x only).

Un-RTBC'ing for 9.3 to signal current status correctly.

I won't get to this today, but I will next week. Fine by me if somebody else figures this one out though!

To fix that error, you have to dump the DB on php 7.3

Last time this happened, I took some notes for the future https://www.drupal.org/project/drupal/issues/3275093#comment-14481944

So the DB dump needs updating basically

Thanks!

Hah — after attempting to redo the crazy things @larowlan linked in #55, I realized something: its results were committed to 9.4.x only.

So what if I just cherry-pick those two files instead?

That worked! 🤓

So basically this patch is:

git checkout 9.3.x
curl -O https://www.drupal.org/files/issues/2022-05-12/3259593-44.patch
git apply -3v 3259593-44.patch
git ci -am 44
git checkout 3aa89b9 -- core/modules/system/tests/fixtures/update/drupal-9.3.0.bare.standard.php.gz
git checkout 3aa89b9 -- core/modules/system/tests/fixtures/update/drupal-9.3.0.filled.standard.php.gz
git ci -nam 57

→ only the "bare" dump changes.

Patch applied successfully (wasn't sure about that due to binary diff), so back to RTBC: test should pass on DrupalCI on PHP 7.3 too.

Committed a9a0696 and pushed to 9.3.x. Thanks!