Problem/Motivation

Part of #3270437: [meta] Tasks to deprecate the CKEditor 4 module. BigPipe has a regression test that uses CKEditor 4.

Proposed resolution

Since the regression does not occur with CKEditor 5 (see #8, #10, and #12), move the test to CKEditor 4.

Remaining tasks

NR.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/A

Issue fork drupal-3270835

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

xjm created an issue. See original summary.

xjm opened merge request !1997

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

xjm credited Wim Leers.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Reviewed & tested by the community

Wim wrote this, so I can RTBC it. :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Oh, wow, I did not expect this to happen! 😄

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review

Since the test is a regression test, I'm wondering after changing to use CKEditor 5, if it's still testing what it's supposed to. I'm wondering if that's something we could confirm by reverting the fix from #2698811: BigPipe unnecessarily renders and sends multiple-occurrence placeholders multiple times when using JS — can cause JS errors and checking if it still fails. I'm wondering what should we do if it doesn't fail? I guess it would make sense to make sure that CKEditor works with Big Pipe, but even more valuable would be to ensure that the original bug is not reintroduced 🤔

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

That's why I originally said I'm on the fence whether we truly want to keep this test — it may be fine to just remove it.

I'll let @xjm or somebody else go through the exercise proposed in #8 — I want to continue to be able to RTBC this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@lauriii just pointed out that it is very unlikely that if we do #8, that tests would fail. Because CKEditor 4 has a highly custom "fetch plugin JS files via HTTP requests computed automatically via URL manipulation" logic, which interfered with CKEditor 4 initializing correctly.

CKEditor 5 on the other hand is being loaded using Drupal's default asset library loading strategy.

So it is likely that removing \Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testCommentForm_2698811() would be logical.

xjm’s picture

xjm
she/her
Primary language English
commented

I pushed a commit to remove the handling of multiple occurrences in sendNoJsPlaceholders(). However, getPlaceholderOrder() has been significantly rewritten since the original fix in #2698811: BigPipe unnecessarily renders and sends multiple-occurrence placeholders multiple times when using JS — can cause JS errors, so there's no clean revert really.

xjm’s picture

xjm
she/her
Primary language English
commented

Edit: Based on https://www.drupal.org/pift-ci-job/2346379 it looks like a different test fails if we revert the original fix, but not this one. OK, I'm convinced we can remove it. :)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

So what I think we should do is:

  • Move it to CKEditor 4's namespace in 9.4.x and mark it @group legacy.
  • Remove the test from 10.0.x along with CKE4 when we are ready to.
xjm’s picture

xjm
she/her
Primary language English
commented
Title: Make BigPipe regression test use CKEditor 5 instead of 4 » Move BigPipe's CKEditor 4 regression test to CKEditor module

xjm closed merge request !1997

xjm opened merge request !2018

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Title: Move BigPipe's CKEditor 4 regression test to CKEditor module » Move BigPipe's CKEditor 4 regression test to the CKEditor module
xjm’s picture

xjm
she/her
Primary language English
commented

Checked the full console output on dispatcher to confirm both the old and new tests are running:

17:17:17 Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegression   3 passes                                      

17:20:22 Drupal\Tests\ckeditor\FunctionalJavascript\BigPipeRegression   1 passes

This is ready for review.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Yay! Simplification 🤩 Nice detective work there, @xjm! 🕵️‍♀️

catch’s picture

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

Committed 650da65 and pushed to 10.0.x, and cherry-picked to 9.4.x. Thanks!

  • catch committed 650da65 on 10.0.x 
    Issue #3270835 by xjm, Wim Leers, lauriii: Move BigPipe's CKEditor 4...

  • catch committed 88a909d on 9.4.x 
    Issue #3270835 by xjm, Wim Leers, lauriii: Move BigPipe's CKEditor 4...
catch’s picture

catch
he/him
Primary language English
commented

Nice to get rid of this one eventually!

Status: Fixed » Closed (fixed)

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