Problem/Motivation
The function drupal_load_stylesheet_content() cuts off contents of every file that starts with
@charset "*some_charset*";
till the last occurence of this combination
*not space*";
if there are no spaces before.
In other words, function is intended to remove multiple charset declarations, but removes a charset and a code that codes after, depending on the file contents. In example, this CSS code:
@charset "UTF-8";html{font-family:'sans-serif';}div{font-family:'Comic Sans';}
will be truncated to
}div{font-family:'Comic Sans';}
instead of
html{font-family:'sans-serif';}div{font-family:'Comic Sans';}
In other words, regexp in the common.inc:3700 does not work properly.
Proposed resolution
Regular expression for removing charset declaration in the drupal_load_stylesheet_content() in the the line common.inc:3700 should be fixed.
Now it looks like:
$contents = preg_replace('/^@charset\s+[\'"](\S*)\b[\'"];/i', '', $contents);
Instead it should be
$contents = preg_replace('/^@charset\s+[\'"](\S*?)\b[\'"];/i', '', $contents);
So it is need to make the "*" quantifier lazy just by adding "?" after it.
Remaining tasks
- The patch for D7 is ready for review.
- The patch for D8 is ready for review.
Comment | File | Size | Author |
---|---|---|---|
#25 | d7-test-remove-charset-2040209-25.patch | 1.17 KB | webevt |
#25 | d7-charset-test-and-fix-2040209-25.patch | 1.8 KB | webevt |
#21 | d8-test-remove-charset-2040209-21.patch | 1.43 KB | webevt |
#21 | d8-charset-test-and-fix-2040209-21.patch | 2.12 KB | webevt |
#19 | d8-test-remove-charset-2040209-19.patch | 1.36 KB | webevt |
Comments
Comment #1
webevt CreditAttribution: webevt commentedAdding a patch.
Comment #2
webevt CreditAttribution: webevt commentedSetting "needs review" status.
Comment #3
webevt CreditAttribution: webevt commentedThe same bug has been found in the 8.x-dev branch in the file CssOptimizer.php:158 in the CssOptimizer::processCss() method.
Patch for the 8.x-dev is ready.
Comment #3.0
webevt CreditAttribution: webevt commentedmessage fix (added the word "of")
Comment #4
andypostThis needs test, suppose unittest is enough
Comment #5
webevt CreditAttribution: webevt commentedAdded test for D7. Don't know whether test for D8 is really needed - it already has a CssOptimizerUnitTest:testOptimize() method that possibly can be used for this purpose...
Comment #7
webevt CreditAttribution: webevt commentedChanging version to 7.22 and component to simpletest.module in order to test the patch with test :-)
Comment #8
webevt CreditAttribution: webevt commented#5: d7-test_remove_charset-2040209-5.patch queued for re-testing.
Comment #10
webevt CreditAttribution: webevt commented>> The last submitted patch, d7-test_remove_charset-2040209-5.patch, failed testing.
So the test is failing because charset declaration is not removed properly. It will pass only after the patch from #3 will be applied.
Comment #11
webevt CreditAttribution: webevt commentedAdding the combined patch containing test and fix (according to this article) for D7.
Comment #12
webevt CreditAttribution: webevt commentedThe bug is present in Drupal 7.23 too.
Comment #13
webevt CreditAttribution: webevt commented#11: d7-charset-test-and-fix-2040209-11.patch queued for re-testing.
Comment #14
ygerasimov CreditAttribution: ygerasimov commentedWe need to have patch against 8.x-dev first and then backported to 7.x-dev (not to any stable 7.x).
Changing version to 7.x-dev as I believe it should work.
@WebEvt please create a patch for 8.x-dev.
Comment #15
webevt CreditAttribution: webevt commentedHere is a patch with the test for 8.x-dev and a combined patch containing test and fix.
Comment #17
webevt CreditAttribution: webevt commentedIt seems that I've forgotten to add css asset type in the test.
Here are the new patches. The first contains the test only, so it should fail (without bug fix patch). The second patch contains both test and fix, so it should pass.
Comment #18
andypostPatch looks RTBC, just a small nitpick
Suppose better to add another case for 'data' => '@charset "UTF-8"' . "\n" ....
Comment #19
webevt CreditAttribution: webevt commentedThanks for your review, andypost!
Added another case to the test, patches are attached (just the test and the test + fix).
Regards
Comment #21
webevt CreditAttribution: webevt commentedAnd once again I've forgotten to set css asset types in the test :-(
So, the next attempt..
Patches are attached (just the test and the test + fix).
Comment #22
andypostNow it's ready!
Comment #23
alexpottCommitted 1baad8a and pushed to 8.x. Thanks!
There's some unnecessary whitespace at the end of the line
'type' => 'inline',
- I removed this during commit.Comment #24
andypost@Thanks, Alex!
@WebEvt please re-roll a d7 patch, and check the white-space pointed in #23
Comment #25
webevt CreditAttribution: webevt commented@Thanks, Alex!
@andypost, it's done :-)
Patches are attached (just the test and the test + fix).
Comment #26
webevt CreditAttribution: webevt commentedRemoved the tag
Comment #27
andypostStraight re-roll, great!
Comment #27.0
andypostRemaining tasks
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/94a26aa