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

Contributor tasks needed
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

CommentFileSizeAuthor
#24 interdiff-2276203-18-24.txt690 byteslauriii
#24 css_aggregation_breaks-2276203-24.patch2.4 KBlauriii
#18 css_aggregation_breaks-2276203-18.patch2.48 KBwebflo
#18 css_aggregation_breaks-2276203-18-testonly.patch1.49 KBwebflo
#17 interdiff-11-17.txt642 bytesnlisgo
#17 css_aggregation_breaks-2276203-17.patch1013 bytesnlisgo
#11 fix-file_create_url-querystring_and_fragment-2276203-11.patch1.62 KBkallehauge
#9 fix-file_create_url-querystring_and_fragment-2276203-9.patch1.6 KBSnipon
#4 fix-file_create_url-querystring.patch832 byteswebflo

Comments

webflo’s picture

webflo commented

Maybe we should fix file_create_url() because this problem is not unique to CSS aggregation.

webflo’s picture

webflo commented
Issue summary: View changes
webflo’s picture

webflo commented
Issue summary: View changes
webflo’s picture

webflo commented
Status: Active » Needs work
Issue tags: +Needs tests, +Novice
StatusFileSize
new fix-file_create_url-querystring.patch832 bytes
martin107’s picture

martin107 commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Issue summary: View changes

Updating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....

selfuntitled’s picture

selfuntitled commented

I'm at Drupalcon Austin sprint, I'll try writing a test for this.

selfuntitled’s picture

selfuntitled commented

Well, didn't get to a test written, but ended up fixing documentation on simpletest. This issue is available if anyone else is interested.

Snipon’s picture

Snipon commented
StatusFileSize
new fix-file_create_url-querystring_and_fragment-2276203-9.patch1.6 KB

Good 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).

Status: Needs review » Needs work

The last submitted patch, 9: fix-file_create_url-querystring_and_fragment-2276203-9.patch, failed testing.

kallehauge’s picture

kallehauge commented
StatusFileSize
new fix-file_create_url-querystring_and_fragment-2276203-11.patch1.62 KB

This is the exact same code as @Snipon in #9 but for beta1. Due to changes in the files, the line numbers are changed.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: fix-file_create_url-querystring_and_fragment-2276203-11.patch, failed testing.

webflo’s picture

webflo commented
Status: Needs work » Needs review

Looks like a random failure.

webflo queued 11: fix-file_create_url-querystring_and_fragment-2276203-11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: fix-file_create_url-querystring_and_fragment-2276203-11.patch, failed testing.

nlisgo’s picture

nlisgo commented
Status: Needs work » Needs review
StatusFileSize
new css_aggregation_breaks-2276203-17.patch1013 bytes
new interdiff-11-17.txt642 bytes

I'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.

webflo’s picture

webflo commented
Issue tags: -Needs tests
StatusFileSize
new css_aggregation_breaks-2276203-18-testonly.patch1.49 KB
new css_aggregation_breaks-2276203-18.patch2.48 KB

Extended the UrlRewritingTest. The first patch should show the failures.

webflo’s picture

webflo commented
Issue tags: +frontend

The last submitted patch, 18: css_aggregation_breaks-2276203-18-testonly.patch, failed testing.

kallehauge’s picture

kallehauge commented

I'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"

kallehauge’s picture

kallehauge commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/includes/file.inc
@@ -322,7 +322,19 @@ function file_create_url($uri) {
+        $path .= (strpos($path, '?') !== FALSE ? '&' : '?') . UrlHelper::buildQuery($options['query']);
...
+        $path .= (strpos($path, '#') !== FALSE ? '' : '#' . $options['fragment'] );

Is the strpos really necessary? Looking at UrlHelper::parse() i don't think so.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new css_aggregation_breaks-2276203-24.patch2.4 KB
new interdiff-2276203-18-24.txt690 bytes
webflo’s picture

webflo commented
Status: Needs review » Reviewed & tested by the community

Thank you. This looks cleaner than the previous patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 585e025 and pushed to 8.0.x. Thanks!

  • alexpott committed 585e025 on 8.0.x 
    Issue #2276203 by webflo, lauriii, nlisgo, kallehauge, Snipon: Fixed CSS...

Status: Fixed » Closed (fixed)

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