Problem/Motivation

I created a custom Text Format/Editor profile that uses only two buttons in the CKEditor toolbar: DrupalLink and DrupalUnlink (both of which are part of the plugin \Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalLink). When this Text Format/Editor loads, my browser's developer tools console displays the error message:

/core/assets/vendor/ckeditor/config.js?t=G2T0 404 (Not Found)

The issue appears to be caused because, in the absence of a customConfig property in the CKEditor instance settings, CKEditor attempts to load a config.js file, based on its global default settings. This issue usually doesn't become apparent because most Text Format/Editor profiles use buttons from \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal, which includes the following in its getConfig() method:

    // Reasonable defaults that provide expected basic behavior.
    $config = array(
      'customConfig' => '', // Don't load CKEditor's config.js file.
      'pasteFromWordPromptCleanup' => TRUE,
      'resize_dir' => 'vertical',
      'justifyClasses' => array('text-align-left', 'text-align-center', 'text-align-right', 'text-align-justify'),
      'entities' => FALSE,
    );

If a Text Format/Editor does not include any buttons from \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal, however, this configuration is not loaded, and CKEditor attempts to load a config.js file, which doesn't exist as part of the Drupal core build.

Proposed resolution

Set a default value for customConfig for all editor instances by adding it to the settings in \Drupal\ckeditor\Plugin\Editor\CKEditor::getJSSettings():

    $settings += array(
      'toolbar' => $this->buildToolbarJSSetting($editor),
      'contentsCss' => $this->buildContentsCssJSSetting($editor),
      'extraPlugins' => implode(',', array_keys($external_plugin_files)),
      'language' => $display_langcode,
      // Configure CKEditor to not load styles.js. The StylesCombo plugin will
      // set stylesSet according to the user's settings, if the "Styles" button
      // is enabled. We cannot get rid of this until CKEditor will stop loading
      // styles.js by default.
      // See http://dev.ckeditor.com/ticket/9992#comment:9.
      'stylesSet' => FALSE,
      // Don't load CKEditor's config.js file.
      'customConfig' => '',
    );

Remaining tasks

Reviews needed.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#12 2696557-12.patch3.89 KBwim leers
#12 2696557-12-test_only_FAIL.patch2.5 KBwim leers
#5 editor.editor.links_only.yml467 bytesdanmuzyka
#3 filter.format.links_only.yml327 bytesdanmuzyka
#3 editor.editor.links_only.yml485 bytesdanmuzyka
#2 2696557-2.patch1.44 KBwim leers
ckeditor-config_file_404_error.patch608 bytesdanmuzyka

Comments

danmuzyka created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2696557-2.patch1.44 KB

Wow, what a find! Amazing that it's only discovered now :)

Can you please export your text editor's configuration and post it here? That'll make it easier to reproduce.

In the mean time, this is IMO the correct solution.

danmuzyka’s picture

danmuzyka commented
StatusFileSize
new editor.editor.links_only.yml485 bytes
new filter.format.links_only.yml327 bytes

Absolutely! I'm attaching the files, but also including the code below:

 

editor.editor.links_only.yml:

langcode: en
status: true
dependencies:
  config:
    - filter.format.links_only
  module:
    - ckeditor
format: links_only
editor: ckeditor
settings:
  toolbar:
    rows:
      -
        -
          name: Links
          items:
            - DrupalLink
            - DrupalUnlink
  plugins:
    stylescombo:
      styles: ''
image_upload:
  status: false
  scheme: public
  directory: inline-images
  max_size: ''
  max_dimensions:
    width: null
    height: null

 

filter.format.links_only.yml:

langcode: en
status: true
dependencies:
  module:
    - editor
name: 'Links Only'
format: links_only
weight: 0
filters:
  filter_html:
    id: filter_html
    provider: filter
    status: true
    weight: -10
    settings:
      allowed_html: '<a href hreflang>'
      filter_html_help: false
      filter_html_nofollow: false
danmuzyka’s picture

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

By the way, I applied 2696557-2.patch locally and removed my own patch, and I can confirm that the approach in 2696557-2.patch works on my end.

danmuzyka’s picture

danmuzyka commented
StatusFileSize
new editor.editor.links_only.yml467 bytes

I just realized I uploaded the wrong version of my editor.editor.links_only.yml file in the attachment in my previous comment (though the copy pasted into the content of my above comment is correct). Uploading the corrected version here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +rc target

Discussed with @xjm, @effulgentsia and @cottser we agreed that this is a 8.1.x rc target as it is a good bug fix that can not be commited in a patch release.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

that can not be committed in a patch release.

Actually, I don't think that's true. This affects 8.0, 8.1 and 8.2. It's a small bugfix for a pretty crazy edge case: when you configure CKEditor to not use any of the typical features: bold, italics, format dropdown, etc. In other words: if you don't use any of the "absolutely basic" functionality, then you run into this.

In other words: this does not cause any regressions, nor any change in behavior. Except in case you aren't using any of those "absolutely basic" things, in which case you're already seeing this 404, but on top of that, you can also resize horizontally and you're getting HTML entities being created. Because this is not added:

  public function getConfig(Editor $editor) {
    // Reasonable defaults that provide expected basic behavior.
    $config = array(
      'customConfig' => '', // Don't load CKEditor's config.js file.
      'pasteFromWordPromptCleanup' => TRUE,
      'resize_dir' => 'vertical',
      'justifyClasses' => array('text-align-left', 'text-align-center', 'text-align-right', 'text-align-justify'),
      'entities' => FALSE,
      'disableNativeSpellChecker' => FALSE,
    );
    …

This can safely be committed to 8.0, and would be fine to commit to 8.1.1 for example, if this doesn't make it before 8.1.0.

xjm’s picture

xjm
she/her
Primary language English
commented

@Wim Leers, in general, we do not allow classes to extend additional interfaces in patch releases, because this could cause collisions in any extending child classes (even if the chance that someone actually extends the class is very theoretical or unlikely). See https://www.drupal.org/core/d8-bc-policy for details. So this would normally be targeted for a minor version.

However, we agreed on it being worth doing for RC, which should avoid that slight risk of semver break.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Ah, I see. I did not think of that in this context — thanks for the explanation, much appreciated!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Looking at the issue summary - this ought to be testable.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers

Totally agreed, this was RTBC'd a bit prematurely.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2696557-12-test_only_FAIL.patch2.5 KB
new 2696557-12.patch3.89 KB

The last submitted patch, 12: 2696557-12-test_only_FAIL.patch, failed testing.

thpoul’s picture

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

Test looks great, marking RTBC. Thank you!

+++ b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
@@ -160,6 +160,45 @@ function testLoading() {
+    // setting to configure CKEditor's Advanced Content Filter, et cetera.

s/et cetera/etc

Just a nit that could be fixed on commit by the committer

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

There's already a for example so et cetera is not necessary. Committed 6205925 and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
index 9a76ec9..eb5f0f1 100644
--- a/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
+++ b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
@@ -186,10 +186,10 @@ protected function testLoadingWithoutInternalButtons() {
     // Even when no buttons of \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal
     // are in use, its configuration (Internal::getConfig()) is still essential:
     // this is configuration that is associated with the (custom, optimized)
-    // build of CKEditor that Drupal core ships with. It for example tells
+    // build of CKEditor that Drupal core ships with. For example, it configures
     // CKEditor to not perform its default action of loading a config.js file,
-    // to not convert special characters into HTML entities, the allowedContent
-    // setting to configure CKEditor's Advanced Content Filter, et cetera.
+    // to not convert special characters into HTML entities, and the allowedContent
+    // setting to configure CKEditor's Advanced Content Filter.
     $this->drupalLogin($this->normalUser);
     $this->drupalGet('node/add/article');
     $editor_settings = $this->getDrupalSettings()['editor']['formats']['filtered_html']['editorSettings'];

Fixed on commit.

  • alexpott committed 5406914 on 8.2.x 
    Issue #2696557 by Wim Leers, danmuzyka: 404 error for CKEditor config.js...

  • alexpott committed 6205925 on 8.1.x 
    Issue #2696557 by Wim Leers, danmuzyka: 404 error for CKEditor config.js...

Status: Fixed » Closed (fixed)

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