Problem/Motivation

Go to /admin/config/content/formats/manage/restricted_html and enable CKEditor.

Expected
Actual

Caused by #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list, found by bisecting.

Proposed resolution

TBD.

Remaining tasks

TBD.

User interface changes

No more broken UI!

API changes

TBD.

Data model changes

TBD.

CommentFileSizeAuthor
#8 2606460-8.patch2.05 KBWim Leers
#8 2606460-8-FAIL.patch1.19 KBWim Leers
#5 2606460-5.patch1.15 KBalexpott
cke_enable_after.png46.55 KBWim Leers
cke_enable_before.png62.85 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: -rc target triage +rc target

Going out on a wing but I think this bug is definitely an RC target.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented 
 <alexpott> WimLeers: looks we need a string cast somewhere :)

Then it's somewhere in \Drupal\ckeditor\Plugin\Editor\CKEditor::settingsForm() that something goes wrong, I suspect template_preprocess_ckeditor_settings_toolbar().

Wim Leers’s picture

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

Note that this is broken in beta16, RC1, RC2 and HEAD.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2606460-5.patch1.15 KB

This fixes the problem - now we need tests.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented 
+++ b/core/modules/ckeditor/ckeditor.admin.inc
@@ -48,6 +48,7 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
+          //$group_name = (string) $group['name'];

I think this line sneaked in?

Wim Leers’s picture

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

Yeah, must be.

Thanks Alex!

Working on test coverage.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » Unassigned
Issue tags: -Needs tests
FileSize
2606460-8-FAIL.patch1.19 KB
2606460-8.patch2.05 KB

The last submitted patch, 8: 2606460-8-FAIL.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Beauty eh. Fixed and test coveraged.

The last submitted patch, 8: 2606460-8-FAIL.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

Reported, fixed, RC triaged, test coverage'd, RTBC'd… all in exactly 4 hours! :O That must be a record!

Thanks, @alexpott & @joelpittet!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed e55daa3 on 8.0.x 
    Issue #2606460 by Wim Leers, alexpott: Regression caused by #2557113:...

Status: Fixed » Closed (fixed)

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