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:

<?php
  $contents
= preg_replace('/^@charset\s+[\'"](\S*)\b[\'"];/i', '', $contents);
?>

Instead it should be

<?php
  $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

  1. The patch for D7 is ready for review.
  2. The patch for D8 is ready for review.
Files: 
CommentFileSizeAuthor
#25 d7-test-remove-charset-2040209-25.patch1.17 KBWebEvt
FAILED: [[SimpleTest]]: [MySQL] 40,464 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#25 d7-charset-test-and-fix-2040209-25.patch1.8 KBWebEvt
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es).
[ View ]
#21 d8-test-remove-charset-2040209-21.patch1.43 KBWebEvt
FAILED: [[SimpleTest]]: [MySQL] 58,342 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#21 d8-charset-test-and-fix-2040209-21.patch2.12 KBWebEvt
PASSED: [[SimpleTest]]: [MySQL] 58,631 pass(es).
[ View ]
#19 d8-test-remove-charset-2040209-19.patch1.36 KBWebEvt
FAILED: [[SimpleTest]]: [MySQL] 58,650 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#19 d8-charset-test-and-fix-2040209-19.patch2.05 KBWebEvt
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 d8-test-remove-charset-2040209-17.patch1001 bytesWebEvt
FAILED: [[SimpleTest]]: [MySQL] 58,238 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 d8-charset-test-and-fix-2040209-17.patch1.67 KBWebEvt
PASSED: [[SimpleTest]]: [MySQL] 58,235 pass(es).
[ View ]
#15 d8-test-remove-charset-2040209-15.patch974 bytesWebEvt
FAILED: [[SimpleTest]]: [MySQL] 58,234 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#15 d8-charset-test-and-fix-2040209-15.patch1.64 KBWebEvt
FAILED: [[SimpleTest]]: [MySQL] 58,196 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 d7-charset-test-and-fix-2040209-11.patch1.64 KBWebEvt
PASSED: [[SimpleTest]]: [MySQL] 40,242 pass(es).
[ View ]
#5 d7-test_remove_charset-2040209-5.patch1.01 KBWebEvt
FAILED: [[SimpleTest]]: [MySQL] 40,339 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 d8-css_truncate_charset-2040209-3.patch709 bytesWebEvt
PASSED: [[SimpleTest]]: [MySQL] 57,163 pass(es).
[ View ]
#1 css_truncate_charset-2040209-1.patch646 bytesWebEvt
PASSED: [[SimpleTest]]: [MySQL] 40,491 pass(es).
[ View ]

Comments

WebEvt’s picture

StatusFileSize
new646 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,491 pass(es).
[ View ]

Adding a patch.

WebEvt’s picture

Status:Active» Needs review

Setting "needs review" status.

WebEvt’s picture

Version:7.22» 8.x-dev
Priority:Major» Normal
StatusFileSize
new709 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,163 pass(es).
[ View ]

The 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.

WebEvt’s picture

Issue summary:View changes

message fix (added the word "of")

andypost’s picture

Issue tags:+Needs tests

This needs test, suppose unittest is enough

WebEvt’s picture

StatusFileSize
new1.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,339 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Added 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...

Status:Needs review» Needs work

The last submitted patch, d7-test_remove_charset-2040209-5.patch, failed testing.

WebEvt’s picture

Version:8.x-dev» 7.22
Component:base system» simpletest.module
Status:Needs work» Needs review

Changing version to 7.22 and component to simpletest.module in order to test the patch with test :-)

WebEvt’s picture

Issue tags:-Needs tests

Status:Needs review» Needs work
Issue tags:+Needs tests

The last submitted patch, d7-test_remove_charset-2040209-5.patch, failed testing.

WebEvt’s picture

Status:Needs work» Needs review

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

WebEvt’s picture

Issue tags:-Needs tests
StatusFileSize
new1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,242 pass(es).
[ View ]

Adding the combined patch containing test and fix (according to this article) for D7.

WebEvt’s picture

Version:7.22» 7.23

The bug is present in Drupal 7.23 too.

WebEvt’s picture

ygerasimov’s picture

Version:7.23» 7.x-dev

We 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.

WebEvt’s picture

Version:7.x-dev» 8.x-dev
Component:simpletest.module» base system
StatusFileSize
new1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,196 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new974 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,234 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here is a patch with the test for 8.x-dev and a combined patch containing test and fix.

Status:Needs review» Needs work

The last submitted patch, d8-charset-test-and-fix-2040209-15.patch, failed testing.

WebEvt’s picture

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,235 pass(es).
[ View ]
new1001 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,238 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

It 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.

andypost’s picture

Patch looks RTBC, just a small nitpick

+++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
@@ -201,6 +201,23 @@ function testOptimize($css_asset, $expected) {
+      'data' => '@charset "UTF-8";html{font-family:"sans-serif";}',

Suppose better to add another case for 'data' => '@charset "UTF-8"' . "\n" ....

WebEvt’s picture

StatusFileSize
new2.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.36 KB
FAILED: [[SimpleTest]]: [MySQL] 58,650 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Thanks for your review, andypost!

Added another case to the test, patches are attached (just the test and the test + fix).

Regards

Status:Needs review» Needs work

The last submitted patch, d8-charset-test-and-fix-2040209-19.patch, failed testing.

WebEvt’s picture

Status:Needs work» Needs review
StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,631 pass(es).
[ View ]
new1.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,342 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Now it's ready!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 1baad8a and pushed to 8.x. Thanks!

+++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
@@ -201,6 +201,38 @@ function testOptimize($css_asset, $expected) {
+        'asset' => array(
+          'type' => 'inline',          ¶
+          'data' => '@charset "UTF-8";html{font-family:"sans-serif";}',
+          'preprocess' => FALSE,
+        ),

There's some unnecessary whitespace at the end of the line 'type' => 'inline', - I removed this during commit.

andypost’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Needs work
Issue tags:+needs backport to D7

@Thanks, Alex!

@WebEvt please re-roll a d7 patch, and check the white-space pointed in #23

WebEvt’s picture

Component:base system» simpletest.module
Status:Needs work» Needs review
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es).
[ View ]
new1.17 KB
FAILED: [[SimpleTest]]: [MySQL] 40,464 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@Thanks, Alex!

@andypost, it's done :-)

Patches are attached (just the test and the test + fix).

WebEvt’s picture

Issue tags:-needs backport to D7

Removed the tag

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Straight re-roll, great!

andypost’s picture

Issue summary:View changes

Remaining tasks

David_Rothstein’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Fixed

Status:Fixed» Closed (fixed)

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