Problem/Motivation

Discovered at #3282233-17: Ability to configure additional languages (e.g. "bash" or "SQL") for CKEditor 5 CodeBlock plugin.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3284254-8.patch3.4 KBWim Leers
#8 interdiff.txt884 bytesWim Leers
#3 3284254-3.patch2.94 KBWim Leers
#3 interdiff.txt1.65 KBWim Leers
#2 3284254-2.patch1.31 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
3284254-2.patch1.31 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt1.65 KB
3284254-3.patch2.94 KB

The last submitted patch, 2: 3284254-2.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented

FilterHtml doesn't throw an error for that kind of configuration it silently ignores it: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/filt...

Not sure if it's relevant or not, but just pointing out it's a difference of handling between two similar things

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

@nod_ Yep, but that's because FilterHtml stores a setting that is essentially user input. Here, we want to be able to compare HTML restrictions. This gets in the way of that. It's up to the code generating HTMLRestrictions objects to only construct valid "HTML restrictions".

That's why this patch only touches HTMLRestrictions, not FilterHTML. So no existing code paths are affected 👍

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

Won't this change break sites that are using class="*". there's a number of filter tickets I believe open about that.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt884 bytes
3284254-8.patch3.4 KB

@smustgrave Fair point!

To prove that this does not cause a failure in those cases, here's an explicit test case proving that parsing from filter_html-like syntax still works as expected. This patch is only explicitly detecting an internal representation edge case that makes no sense.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

The failure on 10.0.x is an unrelated random failure:

1) Drupal\Tests\book\Functional\BookTest::testGetTableOfContents
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Tested on 10.1

  • Patch applied cleanly
  • Added <p class="*"> to Source Editing field
  • Clicked saved
  • Verified allowed html saves as <p class>
  • Created a node and manually edited source to add different class names to a p tag.
  • Verified classes applied.

Because I saw this in ckeditor4 where the style would override a wildcard charcter

  • Created a style that applies to p tag
  • Added <p class="*"> to Source Editing field
  • Clicked saved
  • Verified allowed html saves as <p class>

Everything seemed to work for me.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks, much appreciated @smustgrave! 😊

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.5.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Backporting to 9.4.x because ckeditor is experimental there and this is a low-rick bug fix.

Committed and pushed 846cce70f4 to 10.1.x and 2bdd1d4a88 to 10.0.x and 1a518cdeb5 to 9.5.x and 46aadfc71a to 9.4.x. Thanks!

  • alexpott committed 846cce7 on 10.1.x 
    Issue #3284254 by Wim Leers, smustgrave, nod_: HTMLRestrictions should...

  • alexpott committed 2bdd1d4 on 10.0.x 
    Issue #3284254 by Wim Leers, smustgrave, nod_: HTMLRestrictions should...

  • alexpott committed 1a518cd on 9.5.x 
    Issue #3284254 by Wim Leers, smustgrave, nod_: HTMLRestrictions should...

  • alexpott committed 46aadfc on 9.4.x 
    Issue #3284254 by Wim Leers, smustgrave, nod_: HTMLRestrictions should...

Status: Fixed » Closed (fixed)

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