Summary
Query strings (eg. ?pjfoi0) should be avoided in URLs of static resources (eg. CSS and JavaScript) as it can cause caching issues.
In some cases, Drupal adds a query string hash to CSS URLs for cache-busting. However, this can be avoided especially when the filename already includes a hash of aggregated CSS contents (eg. css_5r96Ja_jU4_qLNpmi92hAFXhO4WgRT1hcq9JHRbrO5c.css).
Drupal already does this for JavaScript, I propose the same behaviour for CSS.
To reproduce
- At admin/config/development/performance, enable CSS and JS file aggregation
- Visit a page as anon and view source
Actual result
- CSS files have a hash in the filename and in a query string
- JS files have a hash in the filename (but no query string)
This can be seen at https://dri.es:
<link rel="stylesheet" href="/sites/default/files/css/css_5r96Ja_jU4_qLNpmi92hAFXhO4WgRT1hcq9JHRbrO5c.css?pjfoi0" media="all" />
<link rel="stylesheet" href="/sites/default/files/css/css_yCk0ufaMqnZTTM6bcwv4yisq5PMm6-bTMscAKOO0UGY.css?pjfoi0" media="all" />
...
<script src="/sites/default/files/js/js_1ufn5cD4yDhD-AhW0q4sUVahgoOMxgCp7H93OfrTKDM.js"></script>
Expected result
- Both CSS and JS files have a hash in the filename (but no query string)
<link rel="stylesheet" href="/sites/default/files/css/css_5r96Ja_jU4_qLNpmi92hAFXhO4WgRT1hcq9JHRbrO5c.css" media="all" />
<link rel="stylesheet" href="/sites/default/files/css/css_yCk0ufaMqnZTTM6bcwv4yisq5PMm6-bTMscAKOO0UGY.css" media="all" />
...
<script src="/sites/default/files/js/js_1ufn5cD4yDhD-AhW0q4sUVahgoOMxgCp7H93OfrTKDM.js"></script>
Comments
Comment #2
hugovk commentedComment #3
hugovk commentedPatch for 8.6.x.
Comment #4
hugovk commentedPatch for 8.7.x.
Comment #5
cilefen commentedComment #6
olli commentedComment #9
hugovk commentedThis is passing on 8.7.x (#4) but failing on 8.6.x (#3).
There were a lot of changes in this file between 8.6.x and 8.7.x to remove a "filthy IE<10 hack", so let's first review the 8.7.x patch.
Putting this to "Needs review".
Comment #10
hugovk commentedNew 8.6.x patch, uses
if (!isset($css_asset['preprocessed']))like the JS code, instead ofif (!$css_asset['preprocessed']). Thanks @olli!Comment #12
hugovk commentedNew 8.7.x patch, same thing.
Comment #14
hugovk commentedAdded latest patch to Drupal 8.8.x testing, it passes.
Is there anything I can do to move this forward? Thanks!
Comment #15
dhope commentedSubscribing.
In my view this is more than just code cleanup/ beautifying. Google search console complains about not being able to access CSS and JS files when pointed to them with a query string added. Google says they don't like blocked page resources so there is likely SEO impact.
Comment #16
hugovk commentedComment #17
papagrandeI tested this successfully on a production site, and the code looks good.
Comment #19
catchGood change. Confirmed it's the same as the js logic and don't think we need a test for it.
Committed 7cbb0ca and pushed to 8.8.x. Thanks!
Comment #20
wim leersI can't believe we didn't notice this for so long! Thanks for fixing this 👍
Comment #21
hugovk commentedThanks for the test, review and merge!
Comment #22
wim leersHah, this broke the test coverage of the CDN module that I maintain. Easy fix though. And it shouldn't have relied on this implementation detail anyway. See #3071814-6: Drupal 9 readiness / compatibility.