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:
- CKEditor 4 as the text editor
filter_autop
filter_url
If you then switch this to CKEditor 5, you'll get two error messages preventing the switch:
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.
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
- Configure a text format like mentioned above (e.g. install Standard, go to
/admin/config/content/formats/manage/restricted_html
and enable CKEditor 4). - Switch it to CKEditor 5.
Proposed resolution
1️⃣ Disable these filters
Automatically disable enabledFilterInterface::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.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
FundamentalCompatibilityConstraintValidator::checkNoMarkupFilters()
currently treats allFilterInterface::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) andfilter_url
.- 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
orfilter_url
: they're allowed to be used in conjunction with CKEditor 5.API changes
None.
Data model changes
None.
Release notes snippet
None.
Comment | File | Size | Author |
---|---|---|---|
#24 | 3273312-22-9.3.x.patch | 14.82 KB | Wim Leers |
#22 | 3273312-22.patch | 14.83 KB | Wim Leers |
| |||
#22 | interdiff.txt | 5.4 KB | Wim Leers |
#13 | CKE4to5_markup_filters--10.0.x--3273312-13.patch | 9.56 KB | Dom. |
#12 | diff--11-12.txt | 1.6 KB | Dom. |
Comments
Comment #3
Wim LeersComment #4
Wim LeersClarified STR.
Comment #6
Wim Leers@dom independently found the same bug at Drupal Dev Days!
Comment #8
Wim LeersWow, `@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
Comment #9
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedThanks @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 :
Comment #10
andreasderijckeI 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.
Comment #11
Wim LeersDiscussed this in detail yesterday at Drupal Developer Days with:
The solution I proposed in the issue summary (to disable these filters in the text format configuration) is not acceptable. Because:
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
Therefore we need a different solution. I propose to exempt these filters:
FundamentalCompatibilityConstraintValidator::checkNoMarkupFilters()
currently treats allFilterInterface::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) andfilter_url
.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.
Comment #12
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedThose do not exists in current codebae, hence the patch fails apply.
Here too
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.
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
Comment #13
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedHere 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.
Comment #14
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedComment #15
ifrikAdditional 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.
Comment #16
Wim Leers#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.Comment #17
mpp CreditAttribution: mpp at AmeXio for District09 commentedNote 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 usingFilterInterface::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:
Comment #18
Wim LeersComment #19
Wim Leers#17 Correct. And that is very much intentional. The typical examples of
FilterInterface::TYPE_MARKUP_LANGUAGE
are:bbcode
commonmark
filter_markdown
shortcode
textile
xbbcode
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
andfilter_url
because they're in core and we should because their use is A) widespread and B) harmless (as described in #11).Comment #20
ifrikTesting patch #13 with two scenarios
Line break and URL conversions are enabled, and
For a sitebuilder, that works fine like that.
Comment #21
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented@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!
Comment #22
Wim LeersSome of the functional JS tests were using
filter_autop
andfilter_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.
Comment #23
Wim LeersComment #24
Wim LeersComment #26
ifrikThe tests passed so I'll RTBC this
Comment #27
mpp CreditAttribution: mpp at AmeXio for District09 commentedRtbc++
Comment #30
alexpottI 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!
Comment #35
joelpittetI 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...