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.Zen subthemes by default have inline images for drupal_set_message() boxes and such (css/components/misc.css):
/**
* Messages.
*/
.messages,
.messages--status,
.messages--warning,
.messages--error {
background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYA…');
}
But when CSS aggregation is enabled, these images don't appear, because a path is being prepended to the value inside url():
.messages,.messages--status,.messages--warning,.messages--error{background-image:url(components/'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYA…');
The culprit is a regex in _drupal_load_stylesheet() which is not greedy enough:
return preg_replace('/url\(\s*([\'"]?)(?![a-z]+:|\/+)/i', 'url(\1'. $directory, $file);
It's trying to make sure it's not firing against absolute paths, or URIs with a protocol (like data:), but it's not working correctly; in the case of url('relative/path'), the regex will pass with \1 being "url('", but in the case of url('data:foo'), the regex still passes, just with \1 being "url(" (no inner quote). I'm guessing there's some branch earlier in the code that stops this from firing with absolute paths or protocoled URLs already, or else I don't see why this wouldn't have caused more obvious problems…
After trying to prod the regex for a bit to stop it from doing that, I just went ahead and replaced it with one based on the more complex pattern in drupal_build_css_cache(), though I try to still keep the quote style (or lack thereof) in the original intact just as the original code does.
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | css-aggregator-2142441-29-d7.patch | 8.99 KB | Garrett Albright |
| #29 | css-aggregator-test-only-2142441-29-d7.patch | 8.39 KB | Garrett Albright |
| #11 | 2132441-11_css_data_paths_aggregation-d8.patch | 6.04 KB | Garrett Albright |
Comments
Comment #1
Garrett Albright CreditAttribution: Garrett Albright commentedComment #2
Garrett Albright CreditAttribution: Garrett Albright commentedComment #3
Garrett Albright CreditAttribution: Garrett Albright commentedAiming for more attention for this by bumping to D8, attaching a D8 patch, and also attaching a D8 subtheme of "stark" which replicates the problem. You should see the Druplicon tile in the background if things work correctly, but you won't before applying the patch.
To clarify the problem, as the theme demonstrates, this happens if you have a data: URL in a CSS file which is @included from another CSS file. If you have a data: URL in a CSS file which is added any other way, there's no problem.
Comment #4
Garrett Albright CreditAttribution: Garrett Albright commentedReassigning to CSS because perhaps it's watched more than "other."
Comment #5
tim.plunkettThis should be testable, right?
Comment #6
Garrett Albright CreditAttribution: Garrett Albright commentedA test, and a re-roll which includes it.
Comment #7
tim.plunkettComment #9
tim.plunkettProtip: If you order them in the opposite order, it won't mark the issue needs work.
I don't know enough about our aggregation code or expectations to RTBC, but the test fails make sense.
Comment #10
Wim LeersLooks great! :) Just some small remarks:
Why not just refer to "data URIs" instead of "path of an inline image"?
Could you please test with a smaller data URI? :) That makes the tests easier to verify.
The smallest one known to mankind: http://stackoverflow.com/a/13139830/80305
url("data:…")case, but I'd also like to see tests for theurl('data:…')andurl(data:…)cases.Comment #11
Garrett Albright CreditAttribution: Garrett Albright commentedOkay, here's examples with all three quote styles, with teensy images (well, almost… thanks, JPEG).
Interestingly, this reveals that this bug isn't actually happening with data URIs which aren't quoted… Here's the buggy output of the relevant line. Note that the .data .no-quote one doesn't have the "../".
Comment #15
RainbowArrayLooks like a newline may be needed?
Comment #17
star-szrI think that newline warning is from the pre-patch file.
Comment #18
RainbowArrayGiven that, and that this still passes, looks RTBC to me.
Comment #19
Wim LeersWonderful, thank you for improving D8's aggregation :)
Comment #20
webchickWow, very nice find! Thanks too for the tests so we don't break this again. :)
Committed and pushed to 8.x. Thanks!
Comment #22
alexpottCommitted 42570bb and pushed to 8.0.x. Thanks!Edit: Nope @webchick committed this one :)
Comment #23
Garrett Albright CreditAttribution: Garrett Albright commentedThis problem is also still present in D7. I'll try to get a D7 patch ready over the weekend, if not later today, if nobody beats me to it.
Comment #24
Garrett Albright CreditAttribution: Garrett Albright commentedAngie, I noticed that you seem to have accidentally committed an older patch to the repo - #6, instead of #11. As per xjm's instructions in #drupal-contribute, here's a diff between the current state of the repo and where it would be had #11 been committed instead.
Comment #25
chx CreditAttribution: chx commentedHere's what I did:
Likely the patch was created this way, even 'cos they are the same byte-to-byte.
Comment #27
webchickOops. Dunno what I was doing there. :P Thanks for the new patch.
Committed and pushed the RIGHT one to 8.x. ;) Thanks!
Comment #28
Garrett Albright CreditAttribution: Garrett Albright commentedw00t! Stoked that we got this fixed before Beta 1.
Okay, back to 7.x we go.
Comment #29
Garrett Albright CreditAttribution: Garrett Albright commentedD7 backport, here we go.
Comment #31
BenStallings CreditAttribution: BenStallings commentedThanks for this, Garrett! Works like a charm.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Comment #35
YesCT CreditAttribution: YesCT commentedComment #36
YesCT CreditAttribution: YesCT commentedchanging to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.