Drupal 10 & CKEditor 5 readiness

1. The "Open in new tab" checkbox can easily be implemented using https://ckeditor.com/docs/ckeditor5/latest/features/link.html#custom-lin...'s manual. But it's probably better to wait for
2. Everything else requires additional infrastructure. Subclassing https://github.com/ckeditor/ckeditor5/blob/v29.2.0/packages/ckeditor5-li... is inadequate, we were told by the CKEditor 5 team.

We'll get more information about this in ~2 weeks.

Open in new window has now been implemented:

So that's #2.1 taken care of.

More news about #2.2 tomorrow, hopefully 😊🤞

Latest news: #3231364-57: Add CKEditor 5 module to Drupal core.

I've been doing some experimentation on this with Oleq from CKSource. We have a PoC that allows adding new form elements to the existing link bubble. Here's how it looks:

The solution works surprisingly well, but requires tinkering with some internal CKEditor properties. I'm assigning this to myself to take the PoC to a MR on this issue.

🥳

@lauriii: Thanks for pushing a working MR! 🥳 Just one more request: can you record a short screencast to show how it works in CKEditor 5, both for this issue and the core issue? 🙏

I'll add the automatic upgrade path if you like, unless you want to do that too?

To test the automatic upgrade path, I modified basic_html to have this allowed HTML:

<br> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <dl> <dt> <dd> <cite> <ul type> <ol start type> <a href rel> <strong> <em> <code> <li> <img src alt data-entity-uuid data-entity-type height width data-caption data-align> <span> <blockquote cite class="interesting"> <p class="callout">

(Note the addition of rel to <a>.)

Before

The Source Editing plugin would get configured to allow setting these additional attributes. But that should not be necessary when Editor Advanced link is installed.

After

Did that in 1eb593bbac9b7dffb2e3512ebd69d49a7053b2de.

While working on this, found #3244911: Automatic upgrade path failing when CKEditor 4 has the StylesCombo plugin configured.

A problem with the current merge request is that it's all or nothing. (Well, for all attributes that are currently supported: title class aria-label.)

That means it currently always looks like this:

But in Drupal 8|9 with CKEditor 4, Editor Advanced Link module just respects the configured HTML restrictions: it only shows "class" if <a class> is allowed. So we should match that! 💪 Right now that works by editor_advanced_link_form_editor_link_dialog_alter() inspecting the HTML restrictions of the text format. That works great. But now we're shifting to a model where this form is rendered client-side, so we can't actually do those checks easily. So we need a different strategy: we need to have the server pass on configuration to the client. IOW: the editorAdvancedLink.EditorAdvancedLink plugin needs to accept configuration.

We could automatically generate that configuration using \Drupal\ckeditor5\Plugin\CKEditor5PluginInterface::getDynamicPluginConfig(), but there's another (related!) catch. See next comment.

The other catch is that in Drupal + CKEditor 5, we're switching things around: rather than requiring the user to manually modify the Filter HTML's Allowed HTML setting (which is very brittle, and requires a certain level of expertise that we should not require), and requiring the user to keep that manually in sync with the CKEditor 5 toolbar/plugin configuration, we're generating that setting automatically.

That also means that the old "configuration" model of the Editor Advanced Link module does not work anymore. It currently instructs users like so:

CONFIGURATION
-------------

Install as usual then:

    - go to the "Text formats and editor" admin page (admin/config/content/formats)
    - configure your text format
    - if the "Limit allowed HTML tags and correct faulty HTML" filter is disabled
      you dont have anything to do with this text format
    - else, add the "title", "class", "id", "target" and/or the "rel" attributes to
      the "allowed HTML tags" field (only those whitelisted will show up in the dialog)

It's that last step that won't work: the user cannot do that.

The solution is simple:

1. introduce CKEditor 5 plugin configuration for the Editor Advanced Link functionality
2. ensure that during the automatic upgrade path, we configure the Editor Advanced Link's plugin configuration in such a way that it matches the text format configuration's allowed attributes on <a>

Step 1 is introducing that configuration UI. I just pushed that. It looks like this:

Step 2: upgrade path. Just pushed that: a81afaa0277790cec14eea4711b99a577c85f6b6.

Wanna see it in action? Yes, I would too:

The only things that remain at this point are:

1. the client side respecting the server-side passed on configuration about which attributes should be enabled. Right now that only works for Open in new window.
2. Supporting the other attributes, other than the ones you can see in the screenshot in #13.

But the key things have been proven:

1. Equivalent (but with better UX!) functionality
2. Equivalent (but with better UX!) configurability
3. Automatic upgrade path

So marking Needs work only for the latter.

Which of the points listed in #16 would be considered needed for this to be mergable into the dev-branch? Are there ideas on how to work on the remaining items?

Bot patch (just info file): #3287149: Automated Drupal 10 compatibility fixes

I'm very excited for this to be merged. Thank you everyone for your hard work on this. I am running into a small issue with running the MR/patch found here. When I try to edit any of the Text Filters I'm getting:

LogicException: The "ckeditor5_link" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "<a href>". in Drupal\ckeditor5\Plugin\CKEditor5PluginManager->getProvidedElements() (line 336 of /var/www/html/web/core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php).

I'm not sure if I missed a patch I needed or if CKEditor5 has updated since this MR was created. If someone could advise me on how to proceed, I would be most appreciated.

Confirmed on Drupal 9.4.5. This is because CKEditor 5 in Drupal core has evolved and improved while it was experimental in the past year 🤓

Note that this merge request was last updated on October 27, 2021, almost a year ago!

Did some digging. This is caused a bug in the filter module that was fixed in #3278636: HTMLRestrictions::fromString() bug: multiple occurrences of same tag results in only last one being respected. ⚠️ That's why you need to update to at least Drupal 9.4.6, to get the required bugfix.

But after that's fixed, we must also update \Drupal\editor_advanced_link\Plugin\CKEditor5Plugin\EditorAdvancedLink::getElementsSubset() because of #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path.

Pushed fix.

But then there's still the need to update the built JS plugin. I know that that's been improved drastically since October last year, but I am not the person most proficient at that. I'll ask @bnjmnm or @lauriii to update that part of the MR!

I just pushed an updated JS build of the plugin and updated build scripts to the current approach.

The editor will not load link.Link due to what I believe is something in editor_advanced_link_ckeditor5_plugin_info_alter

I temporarily set the plugin up as its own plugin instead of extending ckeditor5_link to confirm the built plugin worked, and it worked fine, so with confirmation that the JS itself is good, either the ckeditor5_plugin_info_alter can be changed or the approach can be adjusted to load this as an additional plugin that is only enabled if link.Link also is.

editor_advanced_link_link:
  ckeditor5:
    plugins:
      - editorAdvancedLink.EditorAdvancedLink
    config:
      editorAdvancedLink:
        options:
          - aria-label
          - title
          - class
          - id
          - target
          - rel
  drupal:
    label: Editor advanced link
    class: Drupal\editor_advanced_link\Plugin\CKEditor5Plugin\EditorAdvancedLink
    library: editor_advanced_link/ckeditor5
    elements:
      - <a>
      - <a href aria-label title class id target rel>
    conditions:
      plugins:
        - ckeditor5_link

Thanks, @bnjmnm! But even with that, I get:

ckeditor5.js?riz4yj:137 Failed to load link - Link
(anonymous) @ ckeditor5.js?riz4yj:137
selectPlugins @ ckeditor5.js?riz4yj:127
attach @ ckeditor5.js?riz4yj:208
Drupal.editorAttach @ editor.js?v=9.4.8-dev:172
(anonymous) @ editor.js?v=9.4.8-dev:124
attach @ editor.js?v=9.4.8-dev:112
(anonymous) @ drupal.js?v=9.4.8-dev:27
Drupal.attachBehaviors @ drupal.js?v=9.4.8-dev:24
(anonymous) @ drupal.init.js?v=9.4.8-dev:29
listener @ drupal.init.js?v=9.4.8-dev:17
ckeditor5.js?riz4yj:137 Failed to load link - LinkImage
(anonymous) @ ckeditor5.js?riz4yj:137
selectPlugins @ ckeditor5.js?riz4yj:127
attach @ ckeditor5.js?riz4yj:208
Drupal.editorAttach @ editor.js?v=9.4.8-dev:172
(anonymous) @ editor.js?v=9.4.8-dev:124
attach @ editor.js?v=9.4.8-dev:112
(anonymous) @ drupal.js?v=9.4.8-dev:27
Drupal.attachBehaviors @ drupal.js?v=9.4.8-dev:24
(anonymous) @ drupal.init.js?v=9.4.8-dev:29
listener @ drupal.init.js?v=9.4.8-dev:17
ckeditor5.js?riz4yj:251 CKEditorError: plugincollection-plugin-not-found {"plugin":null}
Read more: https://ckeditor.com/docs/ckeditor5/latest/support/error-codes.html#error-plugincollection-plugin-not-found
    at ckeditor5-dll.js?v=35.1.0:5:518691
    at ckeditor5-dll.js?v=35.1.0:5:518750
    at Array.forEach (<anonymous>)
    at h (ckeditor5-dll.js?v=35.1.0:5:518552)
    at k.init (ckeditor5-dll.js?v=35.1.0:5:517021)
    at R.initPlugins (ckeditor5-dll.js?v=35.1.0:5:524080)
    at editor-classic.js?v=35.1.0:4:9000
    at new Promise (<anonymous>)
    at R.create (editor-classic.js?v=35.1.0:4:8957)
    at Object.attach (ckeditor5.js?riz4yj:220:21)

😢

I posted #23 while #22 was being written! 🙈

or the approach can be adjusted to load this as an additional plugin that is only enabled if link.Link also is.

+1 to that, that's a new capability that didn't exist a year ago! Declarative code FTW.

Implemented your proposal!

Just one bug left?

Looks like we still need to make sure that the JS respects the attributes that actually have been enabled; right now it always shows all of them. 🐛

#3287149: Automated Drupal 10 compatibility fixes just landed, rebasing…

Needs work for Looks like we still need to make sure that the JS respects the attributes that actually have been enabled; right now it always shows all of them. 🐛

I'll tackle upgrade path tests.

The upgrade path completeness test passes just fine. But the upgrade path test itself is uncovering a whole range of problems 😬 Have been investigating, and will have to continue for a while…

Writing complete upgrade path tests is pretty much impossible until #3314770: Enforce order of CKEditor 5 plugin settings in config export (as well as other sequences) is solved.

330a29fb gets us down from 17 failures to 3. For those 3 to pass tests, we need #3314770: Enforce order of CKEditor 5 plugin settings in config export (as well as other sequences).

This now has complete upgrade path test coverage and complete functional JS test coverage.

target="_blank" looks like this:

And all others look like this:

Both of those are a net improvement compared to the CKEditor 4 experience, because they do not require waiting for a modal dialog to be served from Drupal like was the case for CKEditor 4:

Right now, this is only waiting for #3314770: Enforce order of CKEditor 5 plugin settings in config export (as well as other sequences) to land, to allow predictable config export order, which in turn enables maintainable upgrade path tests. Other than that, this is ready for final review.

#3314770: Enforce order of CKEditor 5 plugin settings in config export (as well as other sequences) is in! Pushing a commit to trigger a new test…

D'oh, this is getting tested against 9.4, which doesn't have that fix yet.

So uploading MR as a patch so I can choose to test this against other core versions.

1. 👍 The 9.4 failure looks like predicted.
2. 👍 9.5 is green.
3. 🤓 10 is red, because CKEditor4to5UpgradeCompletenessTest does not exist in D10. That just requires some PHP shenanigans to make it work on all versions.

Done.

On 10.0.x:

$ vendor/bin/phpunit --configuration core/phpunit.xml  modules/editor_advanced_link/tests/src/Kernel/CKEditor4To5Upgrade/UpgradePathCompletenessTest.php
PHPUnit 9.5.24 #StandWithUkraine

Testing Drupal\Tests\editor_advanced_link\Kernel\CKEditor4To5Upgrade\UpgradePathCompletenessTest
S                                                                   1 / 1 (100%)

Time: 00:00.008, Memory: 10.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 0, Skipped: 1.

On 9.5.x:

$ vendor/bin/phpunit --configuration core/phpunit.xml  modules/editor_advanced_link/tests/src/Kernel/CKEditor4To5Upgrade/UpgradePathCompletenessTest.php
PHPUnit 8.5.29 #StandWithUkraine

Testing Drupal\Tests\editor_advanced_link\Kernel\CKEditor4To5Upgrade\CKEditor4to5UpgradeCompletenessTest
...........                                                       11 / 11 (100%)

Time: 13.04 seconds, Memory: 6.00 MB

OK (11 tests, 104 assertions)

On 10:

Drupal\Core\Extension\Exception\UnknownExtensionException: Unknown themes: stable.

… but that's in Drupal\Tests\editor_advanced_link\Functional\AdvancedLinkDialogTest, for CKEditor 4 — not for CKEditor 5. So it's out of scope for this issue.

Still, if you want to fix that, @DuaelFr, it's really simple:

diff --git a/tests/src/Functional/AdvancedLinkDialogTest.php b/tests/src/Functional/AdvancedLinkDialogTest.php
index e1308ca..1d22d31 100644
--- a/tests/src/Functional/AdvancedLinkDialogTest.php
+++ b/tests/src/Functional/AdvancedLinkDialogTest.php
@@ -26,7 +26,7 @@ class AdvancedLinkDialogTest extends BrowserTestBase {
   /**
    * {@inheritdoc}
    */
-  protected $defaultTheme = 'stable';
+  protected $defaultTheme = 'stark';
 
   /**
    * An instance of our custom text format.

Undefined array key "ckeditor" now … because the CKE4 test is relying on the Standard install profile, which has changed …

@DuaelFr I already explained this in the issue summary:

Add to the current major version OR create a new major version that supports both CKE 4 and 5; it's important to support both CKEditor 4 and 5 simultaneously, to allow for an easy transition where both tect editors are supported simultaneously. CKEditor 4 support should be dropped in a new major version.

Supporting CKEditor 4 on Drupal 10 is basically zero effort. I can make the test pass there too for you if that will be the decisive fact.

Argh — #48 does not pass because @requires module FOO only works for kernel tests…

Note that https://git.drupalcode.org/project/editor_advanced_link/-/merge_requests... will actually ensure that after this gets committed, the CKE4 test module will get installed. DrupalCI just refuses to respect dependency-related changes in a patch/MR…

This is really ready … it's only not passing on 9.4 currently because #3314770: Enforce order of CKEditor 5 plugin settings in config export (as well as other sequences) has not been cherry-picked yet. But that only affects the upgrade path test, not the upgrade path itself, nor any of the functionality provided by this module!

This is still ready to go, and has been for a week now 👍

FYI the 2.1.0 release from today doesn't appear to have any of the changes here.

@DuaelFr Hello, I just updated to 2.1.1 and this module broke my whole website:

The website encountered an unexpected error. Please try again later.

🥳