Problem/Motivation

⚠️ Discovered while reviewing #3312442: [PP-1] Make ready for CKEditor 5, which is updating the ckeditor_bidi module to support CKEditor 5 and provide an automatic upgrade path.

There are two places where we do

$end = $can_access_dblog ?
…

in SmartDefaultSettings.

But only the first is preceded by $can_access_dblog = ($this->currentUser->hasPermission('access site reports') && $this->moduleHandler->moduleExists('dblog'));. The second will crash SmartDefaultSettings if the first case wasn't also hit.

This went unnoticed until now because the first case happens in this if-test:

      if (!empty($plugins_enabled) || !$source_editing_additions->allowsNothing()) {

and the second happens in this if-test:

      // Generate warning for:
      // - The addition of <p>/<br> due to them being fundamental tags.
      // - The addition of other tags/attributes previously unsupported by the
      //   format.
      if (!$missing_fundamental_tags->allowsNothing() || !empty($attributes_to_tag) || !empty($added_tags)) {

In Drupal core, the second never happens without the first also happening, because we worked hard to never run into the case of additional attributes or tags being enabled on the text format: if that were to happen, it'd mean that we wouldn't be able to granularly match the HTML enabled for CKEditor 4 plugins!

But … contrib can be different of course: it's possible that it will be less granular in its upgrade path (and hence require supporting additional tags, attributes or attribute values).

Furthermore, it's possible Drupal 9 sites using CKEditor 4 are misconfigured: if the allowed_html is not in sync with the functionality allowed by CKEditor 4, it's possible they did not notice, the upgrade to CKEditor 5 would then automatically fix things for them. Which would then also trigger this warning:

Warning: Undefined variable $can_access_dblog in Drupal\ckeditor5\SmartDefaultSettings->computeSmartDefaultSettings() (line 375 of core/modules/ckeditor5/src/SmartDefaultSettings.php)

Steps to reproduce

See test.

This bug was introduced in #3245967: Messages upon switching to CKEditor 5 are overwhelming.

Proposed resolution

Fix.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#10 3314511-10.patch3.14 KBwim leers
#10 interdiff-4-10.txt628 byteswim leers
#7 3314511-7.patch550 byteswim leers
#7 interdiff.txt628 byteswim leers
#4 3314511-4.patch3.93 KBwim leers
#4 interdiff.txt820 byteswim leers
#3 3314511-3.patch3.16 KBwim leers
#3 interdiff.txt672 byteswim leers
#2 3314511-2.patch3.17 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3314511-2.patch3.17 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt672 bytes
new 3314511-3.patch3.16 KB

D'oh, phpcs mistake…

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt820 bytes
new 3314511-4.patch3.93 KB

Fix.

The last submitted patch, 3: 3314511-3.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major

Bumping priority since literally the next CKE 4 → 5 issue I touched ran into this issue too: #3239667-43: Drupal 10 & CKEditor 5 readiness.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt628 bytes
new 3314511-7.patch550 bytes

Let's run the full core test suite again, now that #3 proved the problem and #4 proved it's easily fixed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

(Not yet queuing a 9.4.x test of #7, that will need a separate patch per the failure in #4. Let's get this first committed to all other branches.)

smustgrave’s picture

smustgrave commented

Appears as though the change in #7 already exists on teh 10.1.x branch. So should #4 be tested?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff-4-10.txt628 bytes
new 3314511-10.patch3.14 KB

D'oh. I messed up #7.

Status: Needs review » Needs work

The last submitted patch, 10: 3314511-10.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

Random fail.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Happened again, for another contrib module: #3232190-81: CKEditor 5 readiness.

Status: Needs review » Needs work

The last submitted patch, 10: 3314511-10.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
smustgrave’s picture

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

With this patch on 9.5 I am able to pass all ckeditor_bidi tests.

Rerunning 10.1.x but willing to mark RTBC for now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.1.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Hopefully eventually PHPStan will be used to detect all of this type of thing.

Committed and pushed 1de949a521 to 10.1.x and 540bf4eaac to 10.0.x and e1621872eb to 9.5.x and 151b5e459c to 9.4.x. Thanks!

diff --git a/core/modules/ckeditor5/src/SmartDefaultSettings.php b/core/modules/ckeditor5/src/SmartDefaultSettings.php
index a9b2e843ce..a10a64b213 100644
--- a/core/modules/ckeditor5/src/SmartDefaultSettings.php
+++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
@@ -280,6 +280,7 @@ public function computeSmartDefaultSettings(?EditorInterface $text_editor, Filte
       }
 
       $help_enabled = $this->moduleHandler->moduleExists('help');
+      $can_access_dblog = ($this->currentUser->hasPermission('access site reports') && $this->moduleHandler->moduleExists('dblog'));
 
       if (!empty($plugins_enabled) || !$source_editing_additions->allowsNothing()) {
         $beginning = $help_enabled ?
@@ -304,7 +305,6 @@ public function computeSmartDefaultSettings(?EditorInterface $text_editor, Filte
             $this->t("Added these tags/attributes to the Source Editing Plugin's Manually editable HTML tags setting: @tag_list", ['@tag_list' => $source_editing_additions->toFilterHtmlAllowedTagsString()]);
         }
 
-        $can_access_dblog = ($this->currentUser->hasPermission('access site reports') && $this->moduleHandler->moduleExists('dblog'));
         $end = $can_access_dblog ?
           $this->t('Additional details are available <a target="_blank" href=":dblog_url">in your logs</a>.',
             [
@@ -371,7 +371,6 @@ public function computeSmartDefaultSettings(?EditorInterface $text_editor, Filte
             }, ''), " ,"),
           ]
         ) : '';
-        $can_access_dblog = ($this->currentUser->hasPermission('access site reports') && $this->moduleHandler->moduleExists('dblog'));
         $end = $can_access_dblog ?
           $this->t('Additional details are available <a target="_blank" href=":dblog_url">in your logs</a>.',
             [

There's no performance reason to not define this outside the ifs. Fixed this on commit.

  • alexpott committed 1de949a on 10.1.x 
    Issue #3314511 by Wim Leers: CKEditor 4 → 5 upgrade path may trigger...

  • alexpott committed 540bf4e on 10.0.x 
    Issue #3314511 by Wim Leers: CKEditor 4 → 5 upgrade path may trigger...

  • alexpott committed e162187 on 9.5.x 
    Issue #3314511 by Wim Leers: CKEditor 4 → 5 upgrade path may trigger...

  • alexpott committed 151b5e4 on 9.4.x 
    Issue #3314511 by Wim Leers: CKEditor 4 → 5 upgrade path may trigger...

Status: Fixed » Closed (fixed)

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