CSS is truncated when aggregation is turned on [#2040209]

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
#19	d8-charset-test-and-fix-2040209-19.patch	2.05 KB	webevt
#17	d8-test-remove-charset-2040209-17.patch	1001 bytes	webevt
#17	d8-charset-test-and-fix-2040209-17.patch	1.67 KB	webevt
#15	d8-test-remove-charset-2040209-15.patch	974 bytes	webevt
#15	d8-charset-test-and-fix-2040209-15.patch	1.64 KB	webevt
#11	d7-charset-test-and-fix-2040209-11.patch	1.64 KB	webevt
#5	d7-test_remove_charset-2040209-5.patch	1.01 KB	webevt
#3	d8-css_truncate_charset-2040209-3.patch	709 bytes	webevt
#1	css_truncate_charset-2040209-1.patch	646 bytes	webevt

Comments

Comment #1

webevt commented 11 July 2013 at 20:33

Status	File	Size
new	css_truncate_charset-2040209-1.patch	646 bytes

Adding a patch.

Comment #2

webevt commented 11 July 2013 at 20:34

Status:

Active

» Needs review

Setting "needs review" status.

Comment #3

webevt commented 16 July 2013 at 08:07

Version:	7.22	» 8.x-dev
Priority:	Major	» Normal

Status	File	Size
new	d8-css_truncate_charset-2040209-3.patch	709 bytes

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.

Comment #3.0

webevt commented 16 July 2013 at 08:07

Issue summary:

View changes

message fix (added the word "of")

Comment #4

andypost

he/him

Russian

commented 16 July 2013 at 21:12

Issue tags:

+Needs tests

This needs test, suppose unittest is enough

Comment #5

webevt commented 24 July 2013 at 21:31

Status	File	Size
new	d7-test_remove_charset-2040209-5.patch	1.01 KB

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

Comment #6

25 July 2013 at 00:10

Status:

Needs review

» Needs work

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

Comment #7

webevt commented 25 July 2013 at 07:05

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 :-)

Comment #8

webevt commented 25 July 2013 at 07:06

Issue tags:

-Needs tests

#5: d7-test_remove_charset-2040209-5.patch queued for re-testing.

Comment #9

25 July 2013 at 07:49

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

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

Comment #10

webevt commented 26 July 2013 at 19:57

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.

Comment #11

webevt commented 27 July 2013 at 09:35

Issue tags:

-Needs tests

Status	File	Size
new	d7-charset-test-and-fix-2040209-11.patch	1.64 KB

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

Comment #12

webevt commented 21 August 2013 at 07:32

Version:

7.22

» 7.23

The bug is present in Drupal 7.23 too.

Comment #13

webevt commented 21 August 2013 at 07:33

#11: d7-charset-test-and-fix-2040209-11.patch queued for re-testing.

Comment #14

ygerasimov commented 21 August 2013 at 08:00

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.

Comment #15

webevt commented 21 August 2013 at 12:26

Version:	7.x-dev	» 8.x-dev
Component:	simpletest.module	» base system

Status	File	Size
new	d8-charset-test-and-fix-2040209-15.patch	1.64 KB
new	d8-test-remove-charset-2040209-15.patch	974 bytes

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

Comment #16

21 August 2013 at 13:41

Status:

Needs review

» Needs work

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

Comment #17

webevt commented 21 August 2013 at 18:40

Status:

Needs work

» Needs review

Status	File	Size
new	d8-charset-test-and-fix-2040209-17.patch	1.67 KB
new	d8-test-remove-charset-2040209-17.patch	1001 bytes

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.

Comment #18

andypost

he/him

Russian

commented 29 August 2013 at 11:58

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

Comment #19

webevt commented 29 August 2013 at 12:20

Status	File	Size
new	d8-charset-test-and-fix-2040209-19.patch	2.05 KB
new	d8-test-remove-charset-2040209-19.patch	1.36 KB

Thanks for your review, andypost!

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

Regards

Comment #20

29 August 2013 at 14:06

Status:

Needs review

» Needs work

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

Comment #21

webevt commented 29 August 2013 at 15:24

Status:

Needs work

» Needs review

Status	File	Size
new	d8-charset-test-and-fix-2040209-21.patch	2.12 KB
new	d8-test-remove-charset-2040209-21.patch	1.43 KB

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

Comment #22

andypost

he/him

Russian

commented 29 August 2013 at 23:11

Status:

Needs review

» Reviewed & tested by the community

Now it's ready!

Comment #23

alexpott

he/they

English

🇪🇺🌍

commented 3 September 2013 at 15:29

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.