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

  1. At admin/config/development/performance, enable CSS and JS file aggregation
  2. 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

hugovk created an issue. See original summary.

hugovk’s picture

Title: Avoid CSS query strings (like JS) » Avoid query strings for aggregated CSS files (like JS)
hugovk’s picture

Patch for 8.6.x.

hugovk’s picture

Patch for 8.7.x.

cilefen’s picture

Issue tags: +Performance
olli’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Active » Needs review

The last submitted patch, 3: 3019393-8.6.x-avoid-css-query-strings.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 3019393-8.7.x-avoid-css-query-strings.patch, failed testing. View results

hugovk’s picture

Status: Needs work » Needs review

This 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".

hugovk’s picture

New 8.6.x patch, uses if (!isset($css_asset['preprocessed'])) like the JS code, instead of if (!$css_asset['preprocessed']). Thanks @olli!

Status: Needs review » Needs work

The last submitted patch, 10: 3019393-8.6.x-avoid-css-query-strings2.patch, failed testing. View results

hugovk’s picture

Status: Needs work » Needs review
StatusFileSize
new1.3 KB

New 8.7.x patch, same thing.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hugovk’s picture

Added latest patch to Drupal 8.8.x testing, it passes.

Is there anything I can do to move this forward? Thanks!

dhope’s picture

Subscribing.

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.

[site]/core/themes/stable/css/system/components/system-status-report-counters.css?ps2cp4 	Stylesheet 	Sonstige Fehler
hugovk’s picture

Issue tags: +SEO
papagrande’s picture

Status: Needs review » Reviewed & tested by the community

I tested this successfully on a production site, and the code looks good.

  • catch committed 7cbb0ca on 8.8.x
    Issue #3019393 by hugovk, PapaGrande: Avoid query strings for aggregated...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Good 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!

wim leers’s picture

I can't believe we didn't notice this for so long! Thanks for fixing this 👍

hugovk’s picture

Thanks for the test, review and merge!

wim leers’s picture

Hah, 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.

Status: Fixed » Closed (fixed)

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