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.
Problem/Motivation
Enabled CSS Aggregation breaks file urls with Query Strings (in my case generated by sass/compass). The url is encoded twice after running through CssOptimizer::rewriteFileURI.
This example shows the issue.
$url = 'sites/default/files/logo.png?123';
print file_create_url($url);
// Result: 'http://example.com/sites/default/files/logo.png%3F123'
// Expected: 'http://example.com/sites/default/files/logo.png?123'
Proposed resolution
Preserve the query string to enable cache busting with other preprocess libs outside of Drupal.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Add automated tests | Instructions | ||
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff-2276203-18-24.txt | 690 bytes | lauriii |
#24 | css_aggregation_breaks-2276203-24.patch | 2.4 KB | lauriii |
Comments
Comment #1
webflo CreditAttribution: webflo commentedMaybe we should fix file_create_url() because this problem is not unique to CSS aggregation.
Comment #2
webflo CreditAttribution: webflo commentedComment #3
webflo CreditAttribution: webflo commentedComment #4
webflo CreditAttribution: webflo commentedComment #5
martin107 CreditAttribution: martin107 commentedComment #6
dcam CreditAttribution: dcam commentedUpdating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....
Comment #7
selfuntitled CreditAttribution: selfuntitled commentedI'm at Drupalcon Austin sprint, I'll try writing a test for this.
Comment #8
selfuntitled CreditAttribution: selfuntitled commentedWell, didn't get to a test written, but ended up fixing documentation on simpletest. This issue is available if anyone else is interested.
Comment #9
Snipon CreditAttribution: Snipon commentedGood going! This one really bugs me. Especially while dealing with SVG fonts.
I altered the patch a little and added support for fragments and query strings without values (might happen).
Comment #11
kallehauge CreditAttribution: kallehauge commentedThis is the exact same code as @Snipon in #9 but for beta1. Due to changes in the files, the line numbers are changed.
Comment #12
rpayanmComment #14
webflo CreditAttribution: webflo commentedLooks like a random failure.
Comment #17
nlisgo CreditAttribution: nlisgo commentedI've removed the added support for fragments or query strings with empty values as is out of scope and breaks tests. I would suggest that it should be raised as a separate issue if you feel strongly about it.
Can anyone prepare some test cases? Even if you can't write the tests themselves there is a task for someone to prepare some useful test cases.
Comment #18
webflo CreditAttribution: webflo commentedExtended the UrlRewritingTest. The first patch should show the failures.
Comment #19
webflo CreditAttribution: webflo commentedComment #21
kallehauge CreditAttribution: kallehauge commentedI've just tested this patch for safety on the latest dev branch and it works. The code looks like it follows Drupals code standard and after @nlisgo "removed support for fragments or query strings with empty values", the patch also follows the scope of the issue.
I will set the status of this ticket as "tested and reviewed by the community"
Comment #22
kallehauge CreditAttribution: kallehauge commentedComment #23
alexpottIs the strpos really necessary? Looking at UrlHelper::parse() i don't think so.
Comment #24
lauriiiComment #25
webflo CreditAttribution: webflo commentedThank you. This looks cleaner than the previous patch.
Comment #26
alexpottCommitted 585e025 and pushed to 8.0.x. Thanks!