Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Came across a slow running busy site that was re-generating the variables cache in cache_bootstrap. Tracing it back led to noticing that drupal_build_css_cache() was calling variable_set() on a lot of pages. Debugging that, and I noticed that despite the output cache file remaining the same, the $key being used was different. This turned out to be because although the CSS files were in the same order, the 'weight' was different.
An example of the relevant variables on 2 separate pages:
Array (
[key] => c9d2d17ceadfc9b67b52ef260f057246175a91f4d9dc8d1e341897a98245c60b
[file] => public://css/css_7z4p4LzjWe3WS10-kJMZFxLoBlJ0wlOUi3zeH4cvC4E.css
[css] => Array (
[0] => Array (
[group] => -100
[type] => file
[weight] => 0.017
[every_page] =>
[media] => all
[preprocess] => 1
[data] => modules/contextual/contextual.css
[browsers] => Array([!IE] => 1, [IE] => 1)
)
)
)
================
Array(
[key] => 4738f07c9a6ad3d5653ba27b72455797feb53b3c8f1aabd8968c6fe3c97bb227
[file] => public://css/css_7z4p4LzjWe3WS10-kJMZFxLoBlJ0wlOUi3zeH4cvC4E.css
[css] => Array(
[0] => Array(
[group] => -100
[type] => file
[weight] => 0.018
[every_page] =>
[media] => all
[preprocess] => 1
[data] => modules/contextual/contextual.css
[browsers] => Array([!IE] => 1, [IE] => 1)
)
)
)
As you can see, the key (the value used to see if the $css variable has been cached yet) is different, yet the output file remains the same. Attached a relatively simple patch that removes the 'weight' key from the CSS/JS arrays before checking if they have been aggregated already.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | js-aggregation-1404380-20-d7.patch | 3.15 KB | msonnabaum |
| #17 | js-aggregation-1404380-17-d7.patch | 3.17 KB | msonnabaum |
| #14 | js-aggregation-1404380-14-tests.patch | 2.04 KB | xjm |
| #14 | js-aggregation-1404380-14-complete.patch | 3.26 KB | xjm |
| #14 | interdiff.txt | 2.14 KB | xjm |
Comments
Comment #1
sunThanks!
Not sure whether this is the best solution, but yeah, those bogus/wonky weights are already added in drupal_add_js/css(), and it's a fairly long way from drupal_add_css() to drupal_get_css() to drupal_build_css_cache().
We need to prevent this from happening again through a test. Simply enabling aggregation and adding and getting/rendering the same files two times, but merely with a different drupal_add_css/js() call order, should be sufficient.
Comment #2
tim.plunkettThis sounds related to, if not duplicating, #1388546: Adding CSS or JS files twice changes weight.
Which I postponed on #575298-94: Provide non-PHP way to reliably override CSS
Comment #3
mjpa commented@tim.plunkett as noted in IRC, that issue about the weights changing is related and more of a trigger for this issue. Without testing, I'm pretty sure it's still possible for the weights to change even without adding the same files twice.
If the last 2 CSS files are foo.css and bar.css but foo.css is set so it's not aggregated, bar.css will be aggregated by itself. However, if the CSS files added before foo.css change in number, bar.css' weight will be different but still be by itself when aggregated (if that makes sense?).
Comment #4
sunRight, the issues referenced in #2 do not seem to be related to this issue.
Restoring tags.
Comment #5
msonnabaum commentedHere's a slightly different approach.
From what I can tell, the only thing relevant for the hash is the files and the order they are in. This patch creates a new array of just the file paths and hashes that instead.
Comment #6
sunSo the only thing that's missing here are proper inline code comments that explain why the additional processing is needed.
And of course, tests, that prove 1) the bug exists, and 2) this patch fixes the bug.
Comment #7
msonnabaum commentedNew version with comments, still needs tests.
Comment #8
msonnabaum commentedHere's a D7 version as well.
Comment #9
xjmNR for the bot to test #7. (#8 is named using the old pattern so it'll be tested and fail whenever this issue is set to NR anyway. The name suffix to use now is
-do-not-test.patch.)From #3:
More detailed steps to reproduce this would help with writing a test.
Comment #11
msonnabaum commentedHere's a new version to retest, only changes are grammer fixes in the comments.
For a test, you'd just have to trigger a page load normally, then trigger it again doing a drupal_add_js/css in hook_init. That should throw off the weights of at least one or more the aggregate groups, and those groups should get a new aggregated file, even though the css/js that was added is not in that group.
Comment #12
msonnabaum commentedAnd here's one with a test.
Comment #13
xjmPolishing a couple things and adding some inline docs.
Comment #14
xjmAttached:
t()from the assertion message text. Reference: http://drupal.org/simpletest-tutorial-drupal7#tI'm also uploading the test in a patch by itself to expose the expected failures on QA.
Comment #15
tim.plunkettThis looks great, especially with the addition of the latest inline docs.
Comment #16
catchOK I think this is fine, and thanks for the nice tests. I don't think #1388546: Adding CSS or JS files twice changes weight is a duplicate - I've seen dependent JavaScript moved after js that it depends on and even into different aggregates due to the automatic weight changing due to repeated calls to drupal_add_js() on a page, but we have that open so we don't need to fix it in this issue.
Committed/pushed to 8.x, moving to 7.x for backport.
Comment #17
msonnabaum commentedHere's a re-roll for D7.
Comment #18
msonnabaum commentedComment #19
damien tournoud commentedThe test needs to be ported as well (there is a
config()in there).Comment #20
msonnabaum commentedYup, just caught that.
Comment #21
xjmBackport looks correct now.
Comment #22
webchickWow. Nice catch, and nice tests. Performance FTW.
Committed and pushed to 7.x. Thanks! This is probably worth calling out in the 7.13 release notes.
Comment #23
wim leersVery nice work, @msonnabaum :)