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

  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

FileSize
646 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
FileSize
709 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

FileSize
1.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
FileSize
1.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
FileSize
1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,196 pass(es), 1 fail(s), and 0 exception(s). View
974 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
FileSize
1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,235 pass(es). View
1001 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

FileSize
2.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 1 fail(s), and 0 exception(s). View
1.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
FileSize
2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,631 pass(es). View
1.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
FileSize
1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es). View
1.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.