CSS files encoded in UTF-8 with BOM break the design when enabling CSS aggregation

Alternatively, shouldn't the aggregator just strip any BOM from all files it includes?

This does sound like a bug in the aggregation routines. Anyway, whether we fix it there or in the documentation, it needs to be addressed in 8.x first then backported to 7.x.

And... can someone please attach a CSS file with a BOM in it (I have no idea how to make one) so that we can create a test for this? Thanks!

forgot backport tag

Considering how very little reaction there has been to this, I think it's safe to assume that almost nobody runs into this problem. Probably because it's recommended in the specs to *not* have the BOM.

If you reopen, please provide a patch that fixes it along with test coverage.

You probably can't see it in your browser, but this patch adds a <U+FEFF> BOM to one of the test files, and produces test failures.

There's a weird seemingly random test failure. But there's also this:

Drupal\Tests\Core\Asset\CssOptimizerUnitTest                   8 passes   1 fails

which indeed proves it's a problem.

grendzy, are you planning to work on a patch to fix this?

I'll give it a shot. A few notes:

• Be careful if editing the test CSS files, to not change the encoding. They have unusual encodings on purpose (UTF-8 with BOM, and Latin-9).
• testOptimizeRemoveCharset() is removed, because the new tests cover not only @charset removal, but conversion as well.
• @charset statements are not legal in a style attribute on an HTML element or inside the <style> element. So, the charset handling is done during file loading, not processCss().

Grendzy, great work that covers the most common cases.

Some remarks:

• Charset names should probably be matched case-insensitive: @charset="utf-8" is the recommended charset for new stylesheets per http://www.w3.org/International/questions/qa-css-charset.en.php. The CSS spec however, refers to preferred names, those are uppercase.
• @charset="" is possible after a BOM. The pregmatch should be anchored on ^BOM, or operate on a stripped BOM
• In certain encodings, '@charset' may not match the ASCII bytesequence '@charset'.

http://www.w3.org/TR/CSS2/syndata.html#charset prescribes match rules (table) to determine the sheet character set. Should that be used for completeness?

Thanks for the review, Heine! I'm very glad you're reviewing this hardcore stuff! :)

Great feedback! I'll make a new patch soon. For the third point, CSS3 seems to have simplified this matching, in that it requires the @charset at-rule to match in hex 40 63 68 61 72 73 65 74 20 22 (not 22)* 22 3B. (ASCII or windows-1252). Would it be acceptable then to not look for @charset rules in UTF-16 and UTF-32, as CSS3 implies this is impossible? (UTF-16 and UTF-32 would still be supported using BOM).

New patch:

• charset case-sensitivity has been addressed. The check is almost unnecessary, it was intended as a performance optimization that proved unwarranted. Micro-benchmarks showed that passing equivalent in/out charsets to convertToUtf8() is essentially no-op. Still, I think it's still possible for convertToUtf8() to fail on systems with no Unicode support, so it's worth checking.
• BOM and @-rule combination has been fixed, and tested.
• Non-ascii-compatible @-rules are ignored, per my understanding of CSS3 spec. However I did add a test with a UTF-16 BOM.

With this change to sass in 3.4+ I think more and more people will be encountering this issue. For me it came up because font-awesome has utf8 in it so sass added a byte-order-mark to that file which then got aggregated in the middle of my other files.

My workaround was to add this sed command to my deploy script (command source is stack overflow question on BOMs:

find sites/all/themes/themename/cssoutputdirectory/ -name '*.css' -exec sed -i '1 s/^\xef\xbb\xbf//' {} \;

Reroll.

Just the tests:

Just some coding style nitpicks here:

1. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -125,6 +126,18 @@ public function loadFile($file, $optimize = NULL, $reset_basepath = TRUE) {
   +      // Check for BOM
   

   Need a period here.

2. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -125,6 +126,18 @@ public function loadFile($file, $optimize = NULL, $reset_basepath = TRUE) {
   +      else if (preg_match('/^@charset "([^"]+)";/', $contents, $matches)) {
   

   This should be elseif.

3. +++ b/core/tests/Drupal/Tests/Core/Asset/css_test_files/css_input_with_bom.css
   @@ -0,0 +1,3 @@
   \ No newline at end of file
   
   +++ b/core/tests/Drupal/Tests/Core/Asset/css_test_files/css_input_with_bom_and_charset.css
   @@ -0,0 +1,4 @@
   \ No newline at end of file
   
   +++ b/core/tests/Drupal/Tests/Core/Asset/css_test_files/css_input_with_charset.css
   @@ -0,0 +1,4 @@
   \ No newline at end of file
   

   Needs to end with a new line.

Assuming #27 goes green, I think this is good to go. It adds a method, so is technically an API change, but it is completely non-breaking.

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -186,6 +186,37 @@ public static function check() {
+   *   The data to be converted. The encoding is detected based on the byte order mark.

+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -125,6 +126,18 @@ public function loadFile($file, $optimize = NULL, $reset_basepath = TRUE) {
+      // If no BOM, check for fallback encoding. Per CSS spec the regex is very strict.

Those should be wrapped at 80 cols.

I really like that we're fixing this - it has caught out front devs on projects I've worked on many times.

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -186,6 +186,37 @@ public static function check() {
   /**
    * Converts data to UTF-8.
...
+  /**
+   * Converts data to UTF-8.

Two methods with the same one line method description? I think it'd be great to be able to discern the difference by reading the one liner.

Also what about javascript aggregation?

Thanks for the reviews! New patch:

• 80-column fix per #31
• Fixed docs per #32. convertToUtf8UsingBOM is now encodingFromBOM – it just returns the encoding - this is more flexible for future users that may not want to load a large file entirely into memory.
• Added JS support for BOM and charset attribute. It's only about 5 lines of actual code to run the same check on JsOptimizer::optimize() (plus tests).
  

Assuming #33 goes green, I think this has addressed the remaining feedback. Back to RTBC.

+++ b/core/tests/Drupal/Tests/Core/Asset/js_test_files/utf8_bom.js.optimized.js
@@ -0,0 +1 @@
+var utf8BOM = '☃';

Snowman FTW!

@grendzy nice changes - really like the the flexibility introduced.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 10c6e1d and pushed to 8.0.x. Thanks!

This would have saved so much aggro and time on Capgemini projects - really happy to see a fix here.

I don't think this was pushed.

We ran into this issue in D7. Attached is the patch I applied, tests still need to be written if someone wants to take a stab.