Problem/Motivation

In #2761233: CKEditor has errors when JS is aggregated a workaround was added for the core/assets/vendor/ckeditor/ckeditor.js path and prevent aggregation for CkEditor. The CkEditor module has been moved to contrib for Drupal 10 (https://www.drupal.org/project/ckeditor). We should probably make the check more flexible to work with CkEditor 5 and the contrib module as well.

Steps to reproduce

  1. Enable the https://www.drupal.org/project/ckeditor module
  2. Enable advagg module and aggregate JS
  3. Create some content with a CkEditor field, the field should be broken.

Proposed resolution

Make the check for the ckeditor.js more generic.

Remaining tasks

User interface changes

-

API changes

-

Data model changes

-

CommentFileSizeAuthor
#6 3310388_6.patch572 bytesghost of drupal past
#2 3310388-2.patch848 bytesseanb

Issue fork advagg-3310388

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

seanB created an issue. See original summary.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Title: Fix erorr in CkEditor » Fix error in CkEditor
Status: Active » Needs review
StatusFileSize
new 3310388-2.patch848 bytes

Patch is attached.

cameron prince’s picture

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

I noticed this bug last week and updated to 5.0 hoping it might fix it, but no dice. This patch does the trick.

Thanks!

tzt20’s picture

tzt20 commented

Same here. Upgraded to 5.0, but this patch did the trick. Thank you!

sd123’s picture

sd123 commented

I managed to workaround this CKEditor 5 related bug without this patch by adding the following lines to the exclusion list in the settings of the MinifyJS module:

core/assets/vendor/ckeditor5/*/*
core/modules/ckeditor5/*
core/modules/ckeditor5/js/*

Thereafter I re-minified all files and deleted all caches.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3310388_6.patch572 bytes

Thanks for the patch.

It didn't work for us but the idea did. So I used the same idea but wrote a shorter and less restrictive version.

klemendev’s picture

klemendev commented

Does this issue apply to cke5 as seems in the patch, or cke4 as referenced in OP?

Enable the https://www.drupal.org/project/ckeditor module

Asking because some of our websites are breaking sometimes with CKE5 (JS not working) and disabling JS minification fixes this

klemendev’s picture

klemendev commented
Priority: Normal » Major
Status: Needs review » Needs work

Ok did some checking. While #6 works, the problem is it removes check for CKE4 JS files

if ($path == 'core/assets/vendor/ckeditor/ckeditor.js') {

so this needs to be re-added.

Changing to major as this randomly breaks any website using CKE5+ADVAGG MinJS

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented

@KlemenDEV, It seems the regex in #6 still keeps the previous check with the "5?", making 5 optional.

So if $path == 'core/assets/vendor/ckeditor/ckeditor.js' then preg_match('#/ckeditor5?\.js$#', $path) still returns true. Isn't that right?

klemendev’s picture

klemendev commented
Status: Needs work » Reviewed & tested by the community

Correct, my bad, missed the question mark. RTBC in this case from my side as patch #6 fixed the problem with CKE5+ADVAGG on our websites.

GasparM’s picture

GasparM commented

It works perfectly, thanks

urvashi_vora made their first commit to this issue’s fork.

urvashi_vora’s picture

urvashi_vora commented

I was facing the same issue with CKEditor and ADVAGG.

Thanks for saving the struggle. Patch 3310388_6.patch worked perfectly.

RTBC++

Committing the patch #6 for helping the maintainers.

urvashi_vora opened merge request !22

joco_sp’s picture

joco_sp commented

#6 worked. Thank you

raghwendra’s picture

raghwendra commented

In version "6.0.0-alpha1" Advagg didn't see the code line that is mention in the patch #6 , therefore not able to apply the patch. Any solutions ckeditor 5 and latest Advagg for Drupal 10 version ?

sleewok’s picture

sleewok commented

Yep, CKEditor is broken with 6.0.x-dev. If aggregation is enabled it will not load.

There is a pending merge request that we are waiting on.

For now here is a patch:

diff --git a/advagg_mod/advagg_mod.module b/advagg_mod/advagg_mod.module
index d2016d0..399a4eb 100644
--- a/advagg_mod/advagg_mod.module
+++ b/advagg_mod/advagg_mod.module
@@ -35,7 +35,7 @@ function advagg_mod_js_alter(&$js) {
if ($config->get('js_preprocess')) {
foreach ($js as $path => &$values) {
// However CKEditor must not be combined or errors *will* occur.
- if ($path == 'core/assets/vendor/ckeditor/ckeditor.js') {
+ if (preg_match('#/ckeditor5?\.js$#', $path)) {
continue;
}
$values['preprocess'] = TRUE;

sd123’s picture

sd123 commented

Neither patch #2, nor the merge request seem to be working. I am using 6.0.0-alpha1 and Drupal 10.0.11 and have the following other patches active:

"Deprecated function: Creation of dynamic property": "https://www.drupal.org/files/issues/2023-07-28/3375725-5.patch",
"Missing *.min.js.map files": "https://git.drupalcode.org/project/advagg/-/merge_requests/21.patch"