Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I added my theme css using the following in the *.info.yml:
ckeditor_stylesheets:
- css/style.css
While updating the css/style.css I found that my changes wasn't reflecting into the WYSIWYG editor. Watching at the html head of the editor iframe I found that my css file was include as follow:
<link type="text/css" rel="stylesheet" href="/my_project/docroot/themes/unepmap/css/style.css">
That is, the above isn't included with any cache busting as any of the other css includes!?...
Now, not sure what is supposed to do CKEDITOR.timestamp included in #2679903: CKEditor uses separate cache-busting query string from Drupal's but it doesn't seems to cover the the above...
Comment | File | Size | Author |
---|---|---|---|
#42 | 2888723-42.patch | 27.32 KB | kim.pepper |
#33 | 2888723-33.patch | 10.35 KB | fenstrat |
Comments
Comment #2
drikc CreditAttribution: drikc commentedThe attached patch add a cache busting into ckeditor iframe css includes.
Comment #3
Wim LeersHah, 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 :)
Rather than calling
\Drupal::state()
, this should inject thestate
service.And rather than making it
?cb=…
, it should be using the same approach as\Drupal\Core\Asset\JsCollectionRenderer::render()
.Comment #5
fenstratAttached addresses the feedback from #3.
Comment #6
Wim Leers👍 — 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:
Comment #9
joshuautley CreditAttribution: joshuautley commentedI 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.
Comment #10
mel-miller CreditAttribution: mel-miller commentedI think I may be having the same problem. Is there a way to manually clear this cache so that I can verify?
Comment #12
johnennew CreditAttribution: johnennew at Deeson commentedHi, Please find a re-roll for Drupal 8.6.x
Also included what I think is being asked for in #6
Comment #13
fenstratComment #15
Petr IllekI've tested the patch on 8.7.1.
Working as expected – styles are updated inside CK Editor.
Also add in coding standards from #14.
Comment #16
Petr IllekForgot to trigger the bot.
Comment #18
Petr IllekTry to fix the test.
Comment #20
Petr IllekHave overlooked the ckeditor_test.css. It wasn't in previous fail reports.
Comment #21
Petr IllekComment #23
Petr IllekAdded the querry parametr to all other css. That should do the trick. :)
Comment #24
kybermanThank you Petr! Everything is fine from my point of view.
Comment #25
fenstratNice one, looks good.
This is an unrelated CS change, but is probably ok to let go through.
Comment #27
yogeshmpawarAll looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).
Comment #29
yogeshmpawarSetting back to RTBC!
This is not happening anymore.
Comment #30
Wim LeersConfirming RTBC.
This matches the logic in
\Drupal\Core\Asset\CssCollectionRenderer
and\Drupal\Core\Asset\JsCollectionRenderer
.Thanks everyone!
Comment #31
alexpottYou can supply a default value to get() -
->state->get(system.css_js_query_string', '0')
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
Comment #32
Wim LeersHah.
\Drupal\Core\Asset\CssCollectionRenderer::render()
does that wrong too then. Let's fix those lines too.A different text editor needs its own implementation, it can't subclass from another text editor's plugin.
This too :)
Comment #33
fenstratAdds 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?
Comment #35
Wim Leersversus
Comment #36
anthony.bouch CreditAttribution: anthony.bouch at Infonomic commentedLook 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...
Hope this helps.
Comment #37
fenstratHere'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?
Comment #38
kim.pepperIt's been a bit of a moving target.
But I believe the current standard format is something like:
Comment #39
fenstrat@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
Comment #40
Amit Dwivedi CreditAttribution: Amit Dwivedi as a volunteer commented@fenstrat Any Idea, how to fix this in Drupal 8.7.x ?
Edit - Patch #23 seems to work with 8.7.7
Comment #42
kim.pepperRe-roll for drupal 8.8.0-beta1
Comment #43
sokru CreditAttribution: sokru as a volunteer commentedManually tested patch from #42 with 8.8.beta1.
Comment #44
alexpottCommitted 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.
I removed the deprecation from the 9.0.x commit. Doesn't need to be there.
Comment #49
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedUnfortunately this patch has broken code like this:
In
buildContentsCssJSSetting()
the code first appends the$query_string
and then callsfile_create_url()
. For a public: pathfile_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.