ckeditor_stylesheets cache busting: use system.css_js_query_string

Hah, nice edge case that you found! Thanks for reporting, and thanks even more for the initial patch, which definitely looks like it's on the right path!

If you can address the following feedback, this will be ready :)

+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -423,6 +423,9 @@ public function buildContentsCssJSSetting(Editor $editor) {
+      return $item . '?cb=' . \Drupal::state()->get('system.css_js_query_string') ?: '0';

Rather than calling \Drupal::state(), this should inject the state service.

And rather than making it ?cb=…, it should be using the same approach as \Drupal\Core\Asset\JsCollectionRenderer::render().

Attached addresses the feedback from #3.

👍 — great step forward! Thank you!

But this is not yet addressed:
And rather than making it <code>?cb=…, it should be using the same approach as \Drupal\Core\Asset\JsCollectionRenderer::render().

That function does this:

          $query_string = $js_asset['version'] == -1 ? $default_query_string : 'v=' . $js_asset['version'];
          $query_string_separator = (strpos($js_asset['data'], '?') !== FALSE) ? '&' : '?';

I spent 3 hours yesterday and around three hours today trying everything I could think of including switching to developer mode over at Cloudflare.

I just not found this thread by searching for "ckeditor_stylesheets" < Sometimes it is just knowing what to search for.

I seriously thought I went crazy after drinking an energy drink. I was like... wait that was working then now, what? Then I went on my six hour trouble shooting binge repeatedly hitting my head again the brick wall until a single brick finally fell out letting me see the light on the other side.

All funny aside. Thank you for working on this. Being able to use our them within the ckeditor is key to helping then end client's staff being able to easily update content and not have to guess how it will look once saved or previewed.

I think I may be having the same problem. Is there a way to manually clear this cache so that I can verify?

Hi, Please find a re-roll for Drupal 8.6.x

Also included what I think is being asked for in #6

I've tested the patch on 8.7.1.
Working as expected – styles are updated inside CK Editor.
Also add in coding standards from #14.

Forgot to trigger the bot.

Try to fix the test.

Have overlooked the ckeditor_test.css. It wasn't in previous fail reports.

Added the querry parametr to all other css. That should do the trick. :)

Thank you Petr! Everything is fine from my point of view.

Nice one, looks good.

+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -205,14 +217,14 @@ public function settingsForm(array $form, FormStateInterface $form_state, Editor
+          'rows' => [
+            0 => [
+              0 => [
+                'name' => 'All existing buttons',
+                'items' => $all_buttons,
+              ],
+            ],
+          ],

This is an unrelated CS change, but is probably ok to let go through.

All looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

Setting back to RTBC!
This is not happening anymore.

Confirming RTBC.

This matches the logic in \Drupal\Core\Asset\CssCollectionRenderer and \Drupal\Core\Asset\JsCollectionRenderer.

Thanks everyone!

+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -423,6 +435,11 @@ public function buildContentsCssJSSetting(Editor $editor) {
+    $query_string = $this->state->get('system.css_js_query_string') ?: '0';

You can supply a default value to get() - ->state->get(system.css_js_query_string', '0')

+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -74,13 +82,16 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, CKEditorPluginManager $ckeditor_plugin_manager, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, RendererInterface $renderer) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, CKEditorPluginManager $ckeditor_plugin_manager, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, RendererInterface $renderer, StateInterface $state) {

Adding a new argument to a constructor can often required adding it will a NULL and then deprecating that code path. We need to validate why we are not doing that here. For me it helps that there is a base class to extend from - \Drupal\editor\Plugin\EditorBase

Hah.

\Drupal\Core\Asset\CssCollectionRenderer::render() does that wrong too then. Let's fix those lines too.

We need to validate why we are not doing that here.

A different text editor needs its own implementation, it can't subclass from another text editor's plugin.

For me it helps that there is a base class to extend from - \Drupal\editor\Plugin\EditorBase

This too :)

Adds the default to state->get(), which also existed in \Drupal\Core\Asset\JsCollectionRenderer::render().

Not sure we need to add $state as a NULL param and deprecate that code path?

            'href' => 'file_url_transform_relative:file_create_url:public://css/file-all?'

versus

            'href' => 'file_url_transform_relative:file_create_url:public://css/file-all?0'

Look forward to this one landing, however, for anyone that discovers this in the meantime, the ckeditor_stylesheets in the theme.info.yml can also be versioned. For example...

ckeditor_stylesheets:
  - css/base/base.css?v=32321321
  - css/components/components.css?v=32321321

Hope this helps.

Here's the fix for CssCollectionRendererUnitTest failures.

I've also included a deprecation notice for the new $state param to the constructor. It follows the format from #3018863: Make SystemMenuBlock's new constructor argument dependency optional.
In the deprecation notice I've just linked to this issue. I'm happy to create a change notice and link to that instead, but not sure if that's standard practice for these deprecations?

It's been a bit of a moving target.

+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -74,13 +82,20 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
+    if ($state === NULL) {
+      @trigger_error('The state service must be passed to CKEditor::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2888723.', E_USER_DEPRECATED);
+      $state = \Drupal::service('state');
+    }

But I believe the current standard format is something like:

    if (!$state) {
      @trigger_error('Calling CKEditor::__construct() without the $state argument is deprecated in drupal:8.8.0. The $state argument is required in drupal:9.0.0. See https://www.drupal.org/node/2888723.', E_USER_DEPRECATED);
      $state = \Drupal::service('state');
    }

@kim.pepper cheers! Attached updates the deprecation message format.

Also rather than just pointing to this issue, I've also gone ahead and created an actual CR https://www.drupal.org/node/3075102

@fenstrat Any Idea, how to fix this in Drupal 8.7.x ?

Edit - Patch #23 seems to work with 8.7.7

Re-roll for drupal 8.8.0-beta1

Manually tested patch from #42 with 8.8.beta1.

Committed bed9f25 and pushed to 9.0.x. Thanks!

Committed and pushed 33b9fe4861 to 8.9.x and 25420c01a6 to 8.8.x. Thanks!

Discussed with @catch and we agreed to backport to 8.8.x as this is a bugfix.

diff --git a/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
index f62825af6f..da8f973ae4 100644
--- a/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -85,16 +85,12 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
    * @param \Drupal\Core\State\StateInterface $state
    *   The state key/value store.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, CKEditorPluginManager $ckeditor_plugin_manager, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, RendererInterface $renderer, StateInterface $state = NULL) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, CKEditorPluginManager $ckeditor_plugin_manager, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, RendererInterface $renderer, StateInterface $state) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->ckeditorPluginManager = $ckeditor_plugin_manager;
     $this->moduleHandler = $module_handler;
     $this->languageManager = $language_manager;
     $this->renderer = $renderer;
-    if ($state === NULL) {
-      @trigger_error('Calling CKEditor::__construct() without the $state argument is deprecated in drupal:8.8.0. The $state argument is required in drupal:9.0.0. See https://www.drupal.org/node/3075102.', E_USER_DEPRECATED);
-      $state = \Drupal::service('state');
-    }
     $this->state = $state;
   }
 
diff --git a/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php b/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
index 47b3ca5685..90148201b4 100644
--- a/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
+++ b/core/modules/ckeditor/tests/src/Kernel/CKEditorTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\ckeditor\Kernel;
 
-use Drupal\ckeditor\Plugin\Editor\CKEditor;
 use Drupal\KernelTests\KernelTestBase;
 use Drupal\language\Entity\ConfigurableLanguage;
 use Drupal\editor\Entity\Editor;
@@ -497,18 +496,4 @@ protected function getDefaultContentsCssConfig() {
     ];
   }
 
-  /**
-   * @deprecationMessage Calling CKEditor::__construct() without the $state argument is deprecated in drupal:8.8.0. The $state argument is required in drupal:9.0.0. See https://www.drupal.org/node/3075102.
-   * @group legacy
-   */
-  public function testConstructorDeprecation() {
-    $editor = new CKEditor([], 'test', ['provider' => 'test'], $this->container->get('plugin.manager.ckeditor.plugin'), $this->container->get('module_handler'), $this->container->get('language_manager'), $this->container->get('renderer'));
-
-    // Ensure the BC layer injects the correct object.
-    $reflection_object = new \ReflectionObject($editor);
-    $reflection_property = $reflection_object->getProperty('state');
-    $reflection_property->setAccessible(TRUE);
-    $this->assertSame($reflection_property->getValue($editor), $this->container->get('state'));
-  }
-
 }

I removed the deprecation from the 9.0.x commit. Doesn't need to be there.

Unfortunately this patch has broken code like this:

/**
 * Implements hook_ckeditor_css_alter().
 */
function XXX_ckeditor_css_alter(array &$css, Editor $editor) {
  $css[] = public://theme/my.css;
}

In buildContentsCssJSSetting() the code first appends the $query_string and then calls file_create_url(). For a public: path file_create_url() seems to escape the ? in the query string to become %3F.

The correct code is to make appending the query string the last stage in this function.

I have raised #3121503: Custom CKEditor CSS is broken with public:// URLs. If any of the authors of this patch could help repair the damage that would be very helpful thanks.