CKEditor uses separate cache-busting query string from Drupal's

Nice! I never even thought about this, nor has it come up at all in the past few years. Even better integration between Drupal 8 and CKEditor.

Thank you very much! :)

Manually tested, works perfectly. Ideally this would get test coverage, but I'm not sure it's worth it in this case.

1. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -100,3 +100,14 @@ function _ckeditor_theme_css($theme = NULL) {
   +  // Pass Drupal's JS cache-busting string via settings along to CKEditor.
   

   Let's link to http://docs.ckeditor.com/#!/api/CKEDITOR-property-timestamp

2. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -100,3 +100,14 @@ function _ckeditor_theme_css($theme = NULL) {
   +  if ($extension == 'ckeditor' && isset($libraries['drupal.ckeditor'])) {
   

   I prefer strict equality.

3. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -273,4 +273,19 @@
   +  Drupal.behaviors.ckeditor = {
   +    attach: function (context, settings) {
   +      // Manually set the cache-busting string to the same value as Drupal.
   +      CKEDITOR.timestamp = settings.ckeditor.timestamp;
   +    }
   +  };
   

   In Drupal 8, the fact that core/ckeditor now has a dependency on core/drupalSettings means that drupalSettings are *guaranteed* to be loaded before core/modules/ckeditor/js/ckeditor.js. Hence this does not need to happen in a Drupal behavior.

   So then it becomes just:

   CKEDITOR.timestamp = drupalSettings.ckeditor.timestamp
   

Actually, this should be quite straightforward to test:

1. Assert using $this->drupalGetSettings() that the setting sent with the page matches the current query string
2. Do something that calls drupal_flush_all_caches()
3. Assert it still matches

Here is a first test.

Thanks, looks great! Just one question (besides the nits below): could you upload a test-only patch to show that the test indeed fails without the actual changes?

1. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -108,7 +108,8 @@ function _ckeditor_theme_css($theme = NULL) {
   +  // http://docs.ckeditor.com/#!/api/CKEDITOR-property-timestamp
   

   Missing @see.

2. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -273,19 +273,7 @@
   +  // Manually set the cache-busting string to the same value as Drupal.
   

   s/Manually set the/Set the CKEditor/

3. +++ b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
   @@ -148,6 +148,16 @@ function testLoading() {
   +    // Remove all caches then make sure that $settings['ckeditor']['timestamp']
   

   s/Remove/Flush/

Here is the test only patch, which should fail the test.

And here are the nits :) Thank you for the review Wim!

Looks perfect!

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Related: #2702171: [upstream] CKEditor's Moono skin does not respect the CKEditor cache-busting query string when loading its icons sprite.

Oops, didn't mean to remove a related issue.