Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Basically, if you're using CKEditor with styles dropdown and "limit allowed HTML tags", automatic "allowed tags" updating can really trip you up.
Steps to reproduce:
- Add a new text format
- Configure it to use CKEditor
- Add "Styles" button to your "Active Toolbar"
- Add a "Styles dropdown" entry, like
p.test|Test
- Enable "Limit allowed HTML tags and correct faulty HTML"
- Under "Allowed HTML tags" enter in
<p class>
- The significance of "class" with no set value is it allows all values
- Save the configuration
- Edit the configuration again
- You see this message:
Based on the text editor configuration, these tags have automatically been added:
<p class=" test">
. - The "Allowed HTML tags" now has
<p class=" test">
This is unnecessary because <p class>
allowed for class='test'
, but is worse because now only the test class is allowed. Of course, you can change this before you save, but you'll have to do it every time you edit the configuration.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#94 | 90-93-interdiff.txt | 1.13 KB | alexpott |
#93 | 2710427-95.x-93.patch | 7.31 KB | alexpott |
#93 | 2710427-95.x-93.test-only.patch | 1.91 KB | alexpott |
#90 | 2710427-95x-89.patch | 7.57 KB | Spokje |
#90 | interdiff_95x_68-89.txt | 480 bytes | Spokje |
Comments
Comment #2
charginghawk CreditAttribution: charginghawk at Genuine commentedComment #3
Wim LeersGood find!
Comment #4
Wim LeersAnd thank you for the very thorough steps to reproduce, that makes this much easier to fix!
Comment #5
nevergone CreditAttribution: nevergone commentedI can reproduce this error.
Comment #8
frobStill an issue
Comment #9
frobThe issue seems to be in this file
core/modules/ckeditor/js/ckeditor.stylescombo.admin.es6.js
This block of code:
I think we should replace that
!_.isEqual()
check with something that instead checks to see if the rules are satisfied. It shouldn't really matter if they are equal.Comment #10
frobSo the logic should really be:
The above is Pseudo code.
_isSatisfied
will need to check to see if the new style rules are in the existing rules.Comment #12
JI_GravityworksI'm also seeing this issue, and it's interfering with our sitewide CSS. We need to both create specific classes that will be available in the Styles dropdown, but also allow all classes on certain tags.
In our case it was working fine until we updated to Drupal Core 9.2.8 today. Now when we correct the tags to remove the specific class declarations (
<h3 class>
) and save the results, the settings revert back show the same tags with restricted classes (<h3 class=" classname" >
). Adding a wildcard doesn't help either (<h3 class=" * classname">
).Are there any suggestions on how we could fix this?
Edited to add:
This does work, but it's less than ideal:
<h3 class="a* b* c* d* e* f* g* h* i* j* k* l* m* n* o* p* q* r* s* t* u* v* w* x* y* z*">
Comment #13
xeM8VfDh CreditAttribution: xeM8VfDh commentedunfortunately, beyond being unnecessary, this issue can be partcularly detrimental for some users: https://www.drupal.org/project/drupal/issues/3248984
Comment #15
akhoury CreditAttribution: akhoury commentedThis patch will ignore the classes provided in the "styles dropdown" as long as the tag added in Allowed HTML field has empty classes i.e
<div class="">
Comment #16
madhaze CreditAttribution: madhaze commentedI tried #15 patch, but it did not work. I have classes set for `p`, `span`, 'div` in the style dropdown. I set the class to `class=""` in the Allowed HTML Tags for them and save. When I edit the text format again and check the Allowed HTML Tags it has put back all the classes from the Styles dropdown.
Comment #17
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan confirm I'm also seeing this and #15 did not work for me either.
Comment #18
smustgrave CreditAttribution: smustgrave at Mobomo commentedI think this could be a major issue because it's forcing users to either abandon ckeditor styles or turn off limiting html tags. This bug is causing issues on several gov sites leveraging uswds library
Comment #19
xeM8VfDh CreditAttribution: xeM8VfDh commentedit is a major issue fr various reasons. I lost styling for various pages as a result.
Comment #20
longwaveComment #21
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo the patch in #15 actually did appear to work
Updated it for 9.3 and allowed for users to put '' or '*'
Comment #23
weseze CreditAttribution: weseze as a volunteer commentedI don't think the check is broken actually, seems to me that is by design and actually doing exactly what it needs to do.
Allowing any classes with the '*' pattern and also forcing to limit and correct HTML seems impossible to me. You can't limit html and then allow every class... It is either limiting html and defining all usable classes, or not limiting HTML.
You can get around this issue by simply defining all classes that you want to allow within the ckeditor.
What was a actual issue, for me at least, is that putting styles on the p-element also requires you to define all classes that can come from other buttons in ckeditor. For example: p.text-align-center comes from the text alignment buttons and that class does not get added automatically.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedThe issue is you are allowed to add class * right now to any tag. But ckeditor styles overrides whatever you have. And some design libraries have 1000s of classes
Comment #25
frob> You can't limit html and then allow every class... It is either limiting html and defining all usable classes, or not limiting HTML.
By doing this we are forcing people who use design libraries to not limit the html and open up the site to XSS. They are being forced because that is the easiest way for this to work. This isn't and impossible problem to solve
<p class>
works already. The problem is changing the setting in the CKEditor styles dropdown overwrites<p class>
to only include the styles from the CKEditor plugin.By design
<p class>
works. CKEditor breaks it.Comment #26
xeM8VfDh CreditAttribution: xeM8VfDh commented@smustgrave has explained why @weseze's analysis is flawed, which is my specific use case here.
Comment #27
charginghawk CreditAttribution: charginghawk commentedJust to piggyback, the Styles dropdown is a convenience, it shouldn't be used to limit classes, just make it easier to add them. Obviously if a class isn't allowed but you add a Style for it, it makes sense to automatically make said class allowed (though honestly you could also just throw a validation error). But with "*" the class should already be allowed so rewriting allowed classes is just a bug.
Comment #28
brhama CreditAttribution: brhama commentedComments
I agree that this can negatively affect a site (happened to mine last month) because the messaging is not clear. If the behavior is by design, which I think it largely is, then there should be a clearer warning about what enabling the "Limit allowed HTML tags..." checkbox will do if you already have classes assigned in your content. The checkbox label, as it is now, says nothing about affecting CSS classes.
Perhaps the label can say, "Limit allowed HTML tags, correct faulty HTML and restrict CSS classes" with help text that includes something like, "Restricts CSS classes to only those listed in the CKEditor Styles Dropdown config above. Warning: enabling this will remove all existing classes from CKEditor content that are not listed above and change the Filter Settings for Limit allowed HTML tags to reflect allowed classes defined above." Something like that.
Possible remediation for affected sites
If you have recently been affected by this bug/issue, there may be a way to restore the original classes to CKEditor content if the node hasn't been edited since the affected date. I just successfully accomplished this on two installations.
I simply disabled the "Limit allowed HTML tags..." checkbox and saved the configuration. All the CKEditor content that had lost styles due to this bug now have said styles again, as long as I haven't edited the node since. This may be due to the fact that the content is still saved in the database with styles intact and CKEditor strips them upon loading the content. I haven't dug that far to confirm this. But it worked for me.
As far as restoring your "Allowed HTML tags" filter settings (which would have been replaced when "Limit allowed HTML tags..." was enabled), I don't know how to retrieve the original value of that field. Hopefully you have a backup copy for that :)
Cheers
Comment #29
alisonI agree with other saying this issue is a bug. The most important part about the HTML filter is to limit allowed tags -- requiring that attribute values get limited in order to use the styles drop-down is overstepping. Optionally, you should be allowed to limit classes and other attribute values by entering allowed values allowed classes, but that should be optional.
A fragile workaround we've got right now is to change the allowed HTML tags setting to allow any classes, like it was before, and save -- it seems to stick that way, until the next time you have to edit that text format.
>> But, I'll codify this change, so as long as we're super careful (risky) not to undo this change, and then I can just config import the settings back.
EDIT: Oops! -- original issue summary already shared the workaround we're doing:
Comment #30
akhoury CreditAttribution: akhoury commentedThis patch is cleaner and will skip updating the "Allowed HTML tags" altogether if the tag has * in the classes list i.e
<p class="*">
Comment #31
akhoury CreditAttribution: akhoury commentedThis patch is cleaner and will skip updating the "Allowed HTML tags" altogether if the tag has * in the classes list i.e
Comment #32
akhoury CreditAttribution: akhoury commentedFixing custom commands failures
Comment #33
alisonDoes anyone have a guess as to why a bunch of us are coming to this issue now / in the last few months?
Is it really just that we all had to upgrade to D9 in November, or did something change in the filter module in the last couple months? I have no proof, but the effects of this issue are REALLY significant for us, and it feels unlikely that no one on our team would've noticed the regressions between early November and now (and actually, most of our sites went to D9 last summer). I will attempt to find out for sure.
-------
(Also changing to NR.)
Comment #34
alisonI wonder if another way to go about fixing this issue could be to change how the filter treats
<tagname class="class1 class2 *">
-- I guess maybe it's icky to have those specific classes superfluously added to the allowed HTML setting, but, if the*
wildcard meant all classes would be permitted, that would be great -- and then the "automatically update allowed_html based on text editor plugin settings" can still do its thing, without causing harm?...Doesn't it seem like a bug that wildcards have the effect you'd expect if "part of" a class name, but not on their own, if the setting for that attribute also includes specific values?
>> For example, if I have
<div class="social-icon button-*">
in the allowed html settings, when I edit a rich text field, the text filter allows<div class="button-link">
, but if I have<div class="social-icon *">
, the text filter strips the "button-link" class.Anyway, I'm not sure I like this idea, but, brainstorming.
-------
Other comments/thoughts from my digging this evening:
(1) It's just clicked for me that the recommended (required?) syntax for allowing a tag with an attribute with no restrictions on the attribute values is
<tagname attribute="*">
, not<tagname attribute>
as many of us have been doing for a while.>> Do y'all agree, or am I misunderstanding?
>> The fix proposed by @akhoury only works if you use the
class="*"
syntax, right? -- if you've got<tagname class>
in your allowed html settings, the proposed fixes aren't going to help you. (And maybe that's okay, because maybe we're supposed to be doingclass="*"
, I just don't actually know.)(2) Speaking of which...
Another manifestation of the issue we're all having, if you are using the
<tagname attribute>
syntax like I was: All tags in "allowed html tags" that hadclass
on them before (notclass="*"
) and that DON'T have any related setting in the styles dropdown are all nowclass=""
.>> BUT, if I changed these instances to
class="*"
, they didn't get messed up when I went to the text format configuration form.(3) I'm running 9.3.6, but I did try updating to 9.3.7 -- no luck (I tried because 9.3.7 does touch FilterHtml.php -- couldn't hurt to check).
>> I also tried the patch on #20 on this issue concerning SmartDefaultSettings -- no dice.
(4) I can't find any recent change to filter.filter_html.admin.es6.js (or ckeditor.stylescombo.admin.es6.js) that could've caused this bug to (re?)surface in the last few months, so probably it's a not-new Drupal 9 issue, and the recent attention is because we all had to upgrade to Drupal 9 a few months ago.
>> For whatever it's worth...
I tried reversing the fix for JS error with elements in "allowed HTML tags" that can't be direct descendants of a div -- before that commit/fix, when I go to configure my text format, all instances of
<tagname class>
were changed to<tagname class="">
-- still broken, just, the JS error meant the stylecombo classes didn't get added.----
Lastly! A reminder / comment of validation for those of us struggling with this issue: Here's the help text for
filter_html
>allowed_html
shown to users on the text format/editor config page (emphasis added):Comment #35
KarlSheaThis seems critical because it will lead to data loss if a node is saved: classes will be stripped permanently from the HTML.
Comment #36
KarlShea> Does anyone have a guess as to why a bunch of us are coming to this issue now / in the last few months?
Something definitely changed very recently, because I had just "
<a class>
" in my allowed HTML settings and it was working on 9.3.0 (held back to not mess up migrations), then I pushed a site live and updated to 9.3.9. Today I needed to adjust some format settings and that's when it broke.Comment #37
scm6079 CreditAttribution: scm6079 commentedAfter operating numerous production sites for many years, this issue has recently started causing data loss on production sites. Any edit to the text format, including simply updating a filter configuration and never activating the tab for allowed HTML tags causes the correct list to be overwritten, and a truncated list to replace it, leading to data loss on page edits.
Changing the allowed HTML tags without prompting, and with no way to keep it from happening is very problematic. At a minimum, the user should be provided with a prompt to OPTIONALLY update the allowed HTML tag list if the issue can not be readily resolved.
Comment #38
aby v a CreditAttribution: aby v a as a volunteer commentedThe given patch showing some undefined errors. I have corrected the condition and updated the patch.
Comment #39
Wim LeersThat has never been correct.
If you want to allow the attribute
bar
on the tagfoo
with no restrictions on which values can be set on that attribute, it's always been<foo bar
. So for example, to allow classes on<div>
s:<div class>
.The only way the asterisk should be used is what the form description says:
So you could say<div data-*>
to allow anydata-
attribute on<div>
s, or you could write<div class="themename-*">
to only allow classes provided by a particular theme.For that reason #31/#38 is definitely wrong.
I think the #1 problem here is that
\Drupal\filter\Plugin\Filter\FilterHtml::settingsForm()
has a horrible UX with no validation whatsoever. So things that seemed to work, do not actually work. 😔 And that is a historical problem that's kinda hard to fix.Reproducing
I just added
to the list of allowed HTML tags. And saved it. I can confirm that this is saved correctly into configuration.
Upon editing, I see this in the UI:
→ this means no classes are allowed at all. This is definitely a critical bug! This also means this must be happening on the client side, in JS.
I looked at recent changes to
core/modules/filter/filter.filter_html.admin.es6.js
and found something that looked suspicious. Reverting that fixes the bug I just mentioned: #2556069: JS error with elements in "allowed HTML tags" that can't be direct descendants of a div.Except … that just brings back the JS error that that fixed:
TypeError: Cannot read property 'tagName' of null in Drupal.behaviors.filterFilterHtmlUpdating
. It was that crash which was preventing this problem from happening 🙃Conclusion
To be able to sanely push this forward, the first thing we need is test coverage. Failing tests. Because I think that #15 is on the right track, but without test coverage it's hard to know for sure.
Comment #40
frobIs there a doc page on how to write js tests for Drupal core?
Comment #41
nevergone CreditAttribution: nevergone commented@frob:
https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/phpunit-...
Comment #43
KarlSheaThis WILL lead to data loss if not resolved with the 9.4.0 release.
Comment #44
xeM8VfDh CreditAttribution: xeM8VfDh commented@KarlShea, it's been leading to data loss for months. Some maintainers may not even know they've been bitten yet.
Comment #45
alisonThanks for the investigation and clarifications @Wim Leers!
Just in case it makes a difference:
The other way we use asterisks is a variation on
<div class="themename-*">
:<div class="intro highlight *">
But, as I think about it, that's probably only because simply using
<div class>
doesn't work, because the next time you're on the page, it gets filled-in based on what's in the "Styles dropdown" setting.So, probably it doesn't matter, but I'm saying it anyway, apparently? 🙃 Y'never know what'll spark something for someone.
Meanwhile, I'll update our internal documentation about the intended/correct way to allow all possible values of an attribute (
<div class>
), so that we're in the right spot later on when the dust settles.Comment #46
ChaseOnTheWebBased on #39, here is a reroll of #15 for 9.5.x, that also fixes the
<div class="">
to be<div class>
.Comment #47
ChaseOnTheWebNovice attempt at writing a test.
Comment #48
smustgrave CreditAttribution: smustgrave at Mobomo commented@ChaseOnTheWeb if you're going to upload a tests-only patch and regular you should include them in the same comment. Uploading the tests-only patch first then the regular. Also your regular patch should include the tests also, ie a full patch. Essentially you want to see the tests-only patch fail and the regular one pass to show a good test case.
Also you can precheck the code for standards before uploading using /core/scripts/dev/commit-code-check.sh
Comment #49
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #52
smulvih2Comment #53
darvanenClosed #2693659: Empty quotes get added to class attribute in "Limit allowed HTML tags" as duplicate of this ticket.
Comment #58
quietone CreditAttribution: quietone at PreviousNext commentedMoving credit from the duplicate to here, as asked for in #bugsmash.
Comment #59
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo removed the code that was checking for class="" but tested manually that
Example 'H2 class' works and if you add a ckeditor style it will add the class to the allowed format correctly too. Example h4 class="test"
Comment #61
LendudePer request in #bugsmash, adding credit for @smulvih2 for work done in closed duplicate #3218499: Allow toggle of automatic update functionality for allowed HTML list
Comment #62
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo removed the code that was checking for class="" but tested manually that
Example 'H2 class' works and if you add a ckeditor style it will add the class to the allowed format correctly too. Example h4 class="test"
Comment #63
smustgrave CreditAttribution: smustgrave at Mobomo commented0 Idea why that resubmitted ignroed #63
Comment #64
smulvih2I created a patch in related ticket from comment #52 above that allows users to disable/enable this feature. I use this patch on a few sites and it works great. I think even if the bug(s) in this ticket are resolved it would be nice to have the ability to disable this feature altogether.
Comment #65
frob@smulvih2 what issue did you create that patch for because that sounds exactly like what I want.
Comment #66
frob@smulvih2 looks like this is the patch https://www.drupal.org/files/issues/2022-05-18/drupal-core-filter-automa...
It would be great if this could get rolled into this issue but I think this should be opened as a feature request in another issue or be rolled into a contrib module somehow.
Comment #67
smulvih2@frob, the issue I am having is when elements are defined in the Styles dropdown, the allowed HTML tags are being updated with these classes, removing the values I had previously defined. For example, if I have a Style defined for
<p>
with a specific class, this automatically updates my allowed HTML list to only allow that specific class. So if my allowed HTML list has<p class>
to allow any class, it is changed to only allow the one class I have defined in my Styles dropdown. This was causing major issues for me across my site. This functionality is coming from core/modules/filter/filter.filter_html.admin.js. My patch allow us to toggle this functionality. I think it's a nice option, and even if the root cause of this issue is fixed I would still want to disable this JS on my sites.Maybe the related ticket from comment #52 should be reopened to allow this toggle, as this current ticket looks to resolve the JS issue; I think both fixes would be nice to have.
https://www.drupal.org/project/drupal/issues/3218499#comment-14519316
Comment #68
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #69
frob@smulvih2
I understand the problem. When I said issue I was referring to the issue node. I was able to find it.
The other issue (the one you posted that patch in originally) was closed as a duplicate and should stay that way. You should open a new feature request for your patch so this one doesn't get any more delayed than it already is. I would recommend opening a new issue for your patch (because I think it is a really good idea) and also see if you can create a contrib module that does what your patch does. It would be more difficult in the contrib space but this issue is over 6 years old and it has become just as likely for CKEditor4 to be removed from core as it is for this issue to be resolved.
Comment #70
nod_nice that it's beeing discussed already, similar issue with CKE5: #3294908: Configuration overlaps between Styles and other CKE5 plugins
Comment #71
SamLerner CreditAttribution: SamLerner at CivicActions for Centers for Medicare and Medicaid Services commentedVerified that the patch in #68 works in Drupal 9.4.3 on several different sites. Thanks for this long-needed fix!
Comment #72
smustgrave CreditAttribution: smustgrave at Mobomo commented@SamLerner if you feel this fixes your issue please review your logs and the patch and mark RTBC so we can maybe get this fix in. Thanks!
Comment #73
SamLerner CreditAttribution: SamLerner at CivicActions for Centers for Medicare and Medicaid Services commentedSetting as RTBC. Thanks again @smustgrave!
Comment #74
catchNeeds a re-roll for 10.x
Comment #75
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot 100% sure this is needed in D10
I added a style for a link "a.test|Link Test"
Then updated "source editing" field to
<a hreflang class>
And the allowed tag is
<a class hreflang href>
Comment #76
longwaveSo in CKEditor 5 the allowed tags field is readonly and controlled by CKEditor 5 itself. But I think we should still commit the change to filter.filter_html.admin.js to 10.0.x to keep it in sync with 9.5.x, even though this codepath is unused in core and I'm not sure that we can test it? But if this had been committed before CKEditor 4 was removed, it would also have made it into 10.0.x.
Comment #77
catchCan't you still have allowed tags in Drupal 10 without ckeditor5 being enabled at all?
Comment #78
smustgrave CreditAttribution: smustgrave at Mobomo commentedHere's a D10.1 patch
Comment #79
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed the test case
Comment #80
longwaveThe test alone from #79 doesn't fail for me, which suggests either the test isn't quite right or the affected codepath is bypassed when CKEditor 5 is used?
Comment #81
Wim LeersThis is correct.
core/modules/editor/js/editor.admin.js
provides the following public JS APIs:Drupal.editorConfiguration
Drupal.EditorFeatureHTMLRule
Drupal.EditorFeature
Drupal.FilterStatus
Drupal.FilterHTMLRule
Drupal.filterConfiguration
Drupal.behaviors.initializeFilterConfiguration
Crucially, none of this has any test coverage, because it predates
FunctionalJavascript
testing infrastructure.Its docs say:
Mea culpa
I wrote this, in #1894644: Unidirectional editor configuration -> filter settings syncing, over 9 years ago. It was how we made the UX for configuring CKEditor 4 somewhat decent: whenever you changed the CKEditor 4 toolbar (added buttons) or changed plugin settings (e.g. configured more styles for
StylesCombo
), it'd automatically update thefilter_html
filter configuration, to keep it in sync.I already knew it was not great. It was a necessary evil. The follow-up work to make it decent (#2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11) never happened.
The reason it's all in JS is simply because for CKEditor 4, it is impossible to know which HTML tags, attributes and attribute values a particular CKEditor capability needs (requires) or (optionally) supports. So … for CKEditor 4, we had a hidden CKEditor 4 instance right in the configuration UI, to query the capabilities "live". That's how we were able to keep
filter_html
in sync. Imperfectly (there are many bug reports around this, for example #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), but … better than nothing.With CKEditor 5, we do know on the server side which HTML tags/attributes/attribute values are supported. This was critical to be able to have an upgrade path at all.
Which means all that infrastructure is unused in Drupal core.
That leaves one key question: does contrib use it at all? Let's find out:
addedFeature
→ 0 resultsremovedFeature
→ 0 resultsmodifiedFeature
→ 0 resultsConclusion: not a single text editor besides core's CKEditor 4 supported this.
drupalEditorFeatureAdded
→ 1 result:linkit
drupalEditorFeatureRemoved
→ 1 result:linkit
drupalEditorFeatureModified
→ 0 resultsConclusion: only one.
Recommendation
Based on this, I would recommend a pragmatic but unusual course of action:
Result: the problem is fixed for CKEditor 4 for as long as that is around (9.4, 9.5, and in contrib during 2023 for 10.0 and 10.1), but not longer (by the time Drupal 10.2 is out on December 13, 2023, CKEditor 4 will not be supported anymore, or will about to be unsupported).
Alternatively, we could remove these in Drupal 10.0 and 10.1 already and require https://www.drupal.org/project/ckeditor to provide a BC layer for them; that way Drupal core can get rid of them sooner.
Comment #82
Wim LeersTangentially related to #81: #3231341: Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 10.1 for removal in Drupal 11.
Comment #83
catchI don't think we should remove these in 10.2 - even if ckeditor4 is unsupported, it's not impossible that someone has it installed and updates to 10.2. Should either be 10.0 or 11.0, but seems like less work to remove in 11.0 - it'll just be dead code for a bit longer but saves re-implementing for ckeditor4.
Comment #84
Wim LeersThat works too.
Repurposed #2567801 — see #2567801-17: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11.
What do you think about recommendations 1 & 2?
Comment #85
longwaveAgree with @catch, we should keep this in 10.x and remove in 11, same as any other deprecated code. Whether we like it or not, some users will be living with CKEditor 4 until the end of life of Drupal 10.
Also agree with committing the fix + test to 9.x and the fix only to 10.x, as we can't write a test without an additional test harness, and even then it would be to support an unused feature. The deprecation can then be committed to 10.1.x or later.
Comment #86
catchYes agreed with #1 and #2 from #81 about skipping the test coverage in 10.x
Comment #87
xjmAgreed with #83 on.
Comment #88
longwaveReuploading #68 for 9.5.x and the fix only from #79 for 10.x.
Comment #90
Spokje@group legacy
to the rescue?Comment #91
longwaveThank you @Spokje - as we are only adding @group legacy to 9.5.x and the test is still present without it in 10.x I think this is OK and the simplest solution to getting this done.
Comment #92
Wim LeersComment #93
alexpottSlightly tweaking the test added on 9.5.x so it is easier to keep things in sync between 10.x and 9.5.x
Comment #94
alexpottHere's an interdiff...
Comment #96
alexpottCommitted and pushed 2aa0817d69 to 10.1.x and 79e1e2a8da to 10.0.x. Thanks!
Committed 52cc95b and pushed to 9.5.x. Thanks!
Comment #100
alexpottDiscussed with @longwave. We agreed to backport this to 9.4.x
Committed 7928ea7 and pushed to 9.4.x. Thanks!