Problem/Motivation

As reported at #3273288: CKE5 and Entity Embed: CKEditor-specific error messages about text filters could be clearer, it's possible to have a text format that was using:

  1. CKEditor 4 as the text editor
  2. filter_autop
  3. filter_url

If you then switch this to CKEditor 5, you'll get two error messages preventing the switch:

  1. CKEditor 5 only works with HTML-based text formats. The "Convert line breaks to HTML (i.e. <br> and <p>)" (filter_autop) filter implies this text format is not HTML anymore.
  2. CKEditor 5 only works with HTML-based text formats. The "Convert URLs into links" (filter_url) filter implies this text format is not HTML anymore.

both of them look like this:

Steps to reproduce

  1. Configure a text format like mentioned above (e.g. install Standard, go to /admin/config/content/formats/manage/restricted_html and enable CKEditor 4).
  2. Switch it to CKEditor 5.

Proposed resolution

1️⃣ Disable these filters

  1. Automatically disable enabled FilterInterface::TYPE_MARKUP_LANGUAGE filters for text formats that are upgrading from CKEditor 4, because then we can be confident that it was already not actively being used.
  2. Inform the user through a warning message.

👆 This is not an acceptable solution because

  • the upgrade path from CKEditor 4 to 5 should not fail due to filter configuration if CKEditor 4 worked fine for these sites, even though it's arguably a nonsensical configuration, but more importantly
  • there actually is a valid use case to have filter_url enabled: @ifrik knows of a site where content creators expect to enter or paste a URL in the text editor, and they expect these to be rendered into clickable links on the front end; @seanB knows of a similar case.

2️⃣ Exempt these filters

  1. FundamentalCompatibilityConstraintValidator::checkNoMarkupFilters() currently treats all FilterInterface::TYPE_MARKUP_LANGUAGE filters the same. But there's two that should be safe to still have enabled (although definitely not recommended): filter_autop (which essentially doesn't do anything when using a text editor) and filter_url.
  2. Result: no more validation error, a working upgrade path! A little bit less perfectionist, but also a lot more pragmatic.

Remaining tasks

  • ✅ Discuss possible solutions with community members.
  • ✅ Implement
  • ✅ Test
  • Review/RTBC 🤓

    User interface changes

    No validation error appears anymore for filter_autop or filter_url: they're allowed to be used in conjunction with CKEditor 5.

    API changes

    None.

    Data model changes

    None.

    Release notes snippet

    None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers credited ifrik.

Wim Leers’s picture

Issue tags: +drupaldevdays
Wim Leers’s picture

Issue summary: View changes

Clarified STR.

Wim Leers credited dom.

Wim Leers’s picture

@dom independently found the same bug at Drupal Dev Days!

Wim Leers credited Dom..

Wim Leers’s picture

Wow, `@Dom.`, not `@dom`, even though d.o redirects https://www.drupal.org/u/Dom. to https://www.drupal.org/u/dom 🤣

@Dom., you're really good at finding edge cases :D

Dom.’s picture

Thanks @Wim Leers
If I can add my two cents on this :
- I thing those kind of filters (FilterUrl and FilterAutoP from the core Filter module) should be disabled in the migration process to CKE5 as stated in "proposed solution" point 1.

But I also feels this weird :

  • It is never said that you should not enable those when using CKEditor 4 so it may be a common scenario
  • It is never said that you should not enable those when using CKEditor 5 either, but we consider it so bad that is fails the upgrade currently
  • Yet they still are on the list : why not hiding them when ckeditor5 is choosen using State API ?
  • Also if uncompatible, why can I still enable them again AFTER the migration to CKE5 and use it again with no trouble ? This makes no sense that it is disable in migration but you can enable it again after
andreasderijcke’s picture

I confirm this case.
The proposed solution sounds fine.

In addition to @Dom's points:
* Would it be possible to disable incompatible filters when CKEditor 5 is active?
* In contrast to @Dom's fourth point, when CKEditor 5 is active, I cannot enable incompatible filters again.
* The current error message is clear, imo.

Wim Leers’s picture

Discussed this in detail yesterday at Drupal Developer Days with:

  • @lauriii
  • @seanB
  • @ifrik
  • @Dom.

The solution I proposed in the issue summary (to disable these filters in the text format configuration) is not acceptable. Because:

  • the upgrade path from CKEditor 4 to 5 should not fail due to filter configuration if CKEditor 4 worked fine for these sites, even though it's arguably a nonsensical configuration (see #1911884: Enable CKEditor in the Standard install profile)
  • there actually is a valid use case to have filter_url enabled: @ifrik knows of a site where content creators expect to enter or paste a URL in the text editor, and they expect these to be rendered into clickable links on the front end; @seanB knows of a similar case.

It's fascinating that so many sites have this enabled: this has been the #1 problem people run into when using the upgrade path on real-world sites, by far, at Drupal Dev Days Ghent 2022! 🤯

We learned that this has usually happened when either

  • creating new text formats and long-time Drupalists are just used to enabling these two filters (no validation happens, so Drupal didn't inform them that this was actually wrong)
  • adding CKEditor (4) to pre-existing text formats that already had these filters enabled

Therefore we need a different solution. I propose to exempt these filters:

  1. FundamentalCompatibilityConstraintValidator::checkNoMarkupFilters() currently treats all FilterInterface::TYPE_MARKUP_LANGUAGE filters the same. But there's two that should be safe to still have enabled (although definitely not recommended): filter_autop (which essentially doesn't do anything when using a text editor) and filter_url.
  2. Result: no more validation error, a working upgrade path! A little bit less perfectionist, but also a lot more pragmatic.

It came as a complete surprise that so many sites had been running CKEditor 4 like this for so many years. Still, being rigid doesn't help them make the move to Drupal 10, so let's be pragmatic. 😊

The attached patch implements this new proposed solution, but only updates kernel tests. I expect some functional JS tests to fail too.

Dom.’s picture

Status: Needs review » Needs work
FileSize
9.55 KB
1.6 KB

Those do not exists in current codebae, hence the patch fails apply.

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
@@ -74,11 +76,12 @@ public function validate($toolbar_item, Constraint $constraint) {
-   * Both of these may be enabled intentionally

Here too

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
@@ -74,11 +76,12 @@ public function validate($toolbar_item, Constraint $constraint) {
-   * Two special cases exist:

This line has changed in 9.4.x, therefore I don't think a unique patch can stand 9.3.x version and the upper ones.

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
@@ -91,6 +94,9 @@ private function checkNoMarkupFilters(FilterFormatInterface $text_format, Fundam
         ->setParameter('%filter_label', (string) $markup_filter->getLabel())

Attached is a new patch that runs on 9.3.x only.

Despite this, I could test it functionally on my website on 9.3.x and it works just fine as expected :
- I could update from CKE4 to 5 with both filters activated
- I could update from CKE4 to 5 with none of those filters activated
- I could update from CKE4 to 5 with one only (filterUrl) of the filter applied
- I could disable / enable again multiple times any combinaison of those filters with CKE5 enabled

Following is an update of the patch for 9.4 and 10.x

Dom.’s picture

Here is again the same patch for 9.4.x and 10.0.x

I updated my website to 9.4.x and tested with the same protocol again that new patch.

Dom.’s picture

Status: Needs work » Needs review
ifrik’s picture

Additional note to comment #11
I was refering to a configuration where CKEditor has been added to one of the text formats (restricted_html), but very few buttons (strong, italics, lists) are available to users.
The Link button is not available, but URLs are converted automatically into links. Disabling the URL convesion would break this.
It's used for comments by anonymous users.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work

#12: thanks for fixing my patch 😊🙈
#15: That's exactly what the patch above is fixing! Could you perhaps try applying #13 and test again? 🙏

I'm working on fixing the last test failure: Drupal\Tests\ckeditor5\FunctionalJavascript\AdminUiTest, which I predicted in #12 would fail.

mpp’s picture

Note that exempting these 2 filters won't solve all cases as there might be custom filter plugins that replace use Drupal\filter\Plugin\FilterInterface::TYPE_MARKUP_LANGUAGE.

For those cases, we should document what steps to take to upgrade.

For instance this patch uses FilterInterface::TYPE_MARKUP_LANGUAGE to replace a token with some markup. This is probably a bug so I fixed it over there by using FilterInterface::TYPE_TRANSFORM_IRREVERSIBLE.

There might be other custom plugins that have a similar issue.

Tested the patch in #13 but when filter_anchor is enabled in CKeditor 4 it still reports this error when migrating to 5:

 CKEditor 5 only works with HTML-based text formats. The "Anchor Filter" (filter_anchor) filter implies this text format is not HTML anymore.
Wim Leers’s picture

Issue tags: -drupaldevdays +ddd2022
Wim Leers’s picture

#17 Correct. And that is very much intentional. The typical examples of FilterInterface::TYPE_MARKUP_LANGUAGE are:

i.e.: text that follows a certain syntax that gets transformed into HTML at filtering (rendering) time.

It is impossible for the CKEditor 5 upgrade path to take arbitrary filters into account. We can special case filter_autop and filter_url because they're in core and we should because their use is A) widespread and B) harmless (as described in #11).

ifrik’s picture

Testing patch #13 with two scenarios

Line break and URL conversions are enabled, and

  • 'p' and 'br' tags are not in the list of allowd HTML tags: Error message is displayed, stating that 'p' and 'br' tags need to be added to the list of Allowed Tags. After that the configuration can be saved.
  • 'p' and 'br' tags are in the list of allowed HTML tags: No validation errors displayed.

For a sitebuilder, that works fine like that.

Dom.’s picture

@ifrik awesome, I forgot this scenario to test !
No pressure on @wim-leers of course, but he is working on the remaining tests so I guess that *could* be RTBC in the drupaldevdays timeframe hopefully!

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
5.4 KB
14.83 KB

Some of the functional JS tests were using filter_autop and filter_url to test edge case scenarios about the admin UI infrastructure, not specific details about those filters. So I changed those tests slightly, to continue to test the same admin UI infrastructure, but without using those two filters.

This should be green.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

The last submitted patch, 22: 3273312-22.patch, failed testing. View results

ifrik’s picture

Status: Needs review » Reviewed & tested by the community

The tests passed so I'll RTBC this

mpp’s picture

Rtbc++

alexpott credited lauriii.

alexpott credited seanB.

alexpott’s picture

Version: 10.0.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

I was wondering about contrib projects like https://www.drupal.org/project/autofloat and what we should be telling them - this one looks quite like the \Drupal\filter\Plugin\Filter\FilterAutoP case to me. Perhaps the answer is the same as #17 - maybe we need to update the CR to be more specific about this situation.

That said I think doing the special case here will work for now. Crediting @lauriii and @seanB due to #11.

Committed and pushed e8ed42cc2f to 10.0.x and 1a834c9eab to 9.4.x and 994805a99b to 9.3.x. Thanks!

  • alexpott committed e8ed42c on 10.0.x
    Issue #3273312 by Wim Leers, Dom., ifrik, mpp, seanB, lauriii: Upgrading...

  • alexpott committed 1a834c9 on 9.4.x
    Issue #3273312 by Wim Leers, Dom., ifrik, mpp, seanB, lauriii: Upgrading...

  • alexpott committed 994805a on 9.3.x
    Issue #3273312 by Wim Leers, Dom., ifrik, mpp, seanB, lauriii: Upgrading...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

joelpittet’s picture

I just noticed one of the sites I maintain (still on D7 but in progress of migrating) yesterday that it filtered <p> and inserted them with the auto linebreak to P filter, more of a note to self but also so you all no it may be common...