Strip UTF-8 BOMs from each file when concatenating

[suyogyashukla]

Fixes #49, reported again in a support ticket.

Review so far

Gary Jones reviewed this on a fork PR first: suyogyashukla#1. He approved 1605230, which is the head commit here, so these are the exact commits he read. He ran the helper standalone on PHP 8.3, checked the substr_compare bounds guard, and measured the loop's scaling independently.

Three things in the text below come from that review: the coupling trade-off in the charset section, the note that the @charset bug predates this change, and the timings reported as a ratio rather than absolutes. His push-back on the multi-BOM rationale produced two comment-only commits.

The problem

A BOM gives the encoding only at offset 0 of a file. The concatenator reads each file and appends the raw bytes, so the BOM of any file after the first becomes a U+FEFF character in the middle of the response. CSS accepts that character in a selector. The browser reads it as part of the next selector and ignores that rule, then carries on with the rest of the file.

The same rule renders with ?concat_css=false. The site that reported it ships a BOM on frontend.css from a Dart Sass build, and lost *{margin:0;padding:0}.

A BOM also breaks the @charset hoist, because that test reads offset 0.

The fix

Remove the BOM after the read and before the code that uses the buffer. relative_path_replace, the @charset hoist, the @import hoist and cssmin all read that buffer, so the position of this code matters. The JS branch gets the same change.

If a BOM was present, add charset=UTF-8 to the Content-Type header. Issue #49 does not ask for this, but the BOM was the encoding declaration for that file, and a strip on its own drops it. A variant that strips and declares nothing, served to a windows-1252 page:

Variant	CSS content char codes	Result
Strip only	[34, 226, 8224, 8217, 34]	"→", wrong
Strip and declare	[34, 8594, 34]	"→" (U+2192), correct

The Content-Type charset has a higher priority than a BOM and than an @charset rule, so the bundle stays UTF-8 for any file position.

Two things about that header, both worth recording. The charset is set only after a BOM, so nothing is claimed about the encoding of bundles that had none. But it also means the header now depends on a byte this code deletes: if a build stops emitting a BOM, the bundle loses charset=UTF-8 and the encoding interpretation changes, with no change to this plugin. The alternatives are to always declare UTF-8 for CSS and JS, which is a larger change than #49 asks for, or to re-emit one BOM at offset 0 and leave the header alone.

The practical scope is smaller than it looks. On VIP, CSS bundles already come back as text/css;charset=utf-8, and JS bundles as application/javascript with no charset, because PHP appends default_charset only to a text/* header. So for CSS on VIP this changes the spelling of a charset that was already there, to text/css; charset=UTF-8, which is the same value under RFC 7231. It carries real weight for JS, and for any deployment with an empty default_charset. PHP does not add a second charset when one is already set.

Testing

This file is the byte path for every concatenated asset request on VIP and WP.com.

I built a document root, drove ngx-http-concat.php over HTTP, and compared this branch against master for 17 requests. Every request without a BOM returns the same bytes, status and headers. Every request with a BOM loses the BOM, and Content-Length matches the body in all 17. The 404 and 400 paths do not change.

tests/test-concat-utils.php gains 46 cases. The suite is 78 tests and passes on PHPUnit 9.6, the version in composer.lock.

For the reported CSS in a browser, master leaves body at the 8px user agent margin and this branch gives 0px.

phpcs --standard=WordPress-VIP-Go reports the same violations on this branch as on master for both changed files, and none on the new test file.

Cases covered: a BOM in the first file, a later file and the only file; more than one BOM; a file of only a BOM; a file shorter than a BOM; an incomplete BOM; a BOM with @charset; a BOM with @import; a UTF-16 BOM; JS in both positions; the gzip and base64 URL form; HEAD; and a subdirectory install.

The cost on the common path is one 3-byte strncmp for each file, because has_utf8_bom returns before the loop starts. A file of only BOMs stays linear: at 50k, 100k and 200k BOMs, each doubling of the count doubles the time. Absolute timings are hardware-specific, so the ratio is the part to read.

Notes

Bundles already in the edge cache do not change. They hold for a year and clear on a version change, so a site with this problem needs a version change or a purge after this deploys.

The @charset hoist has a separate bug, and it predates this change. Files that start with @charset already get the rule copied to $pre_output and left in the body, because the preg_replace_callback result is never assigned. This change makes BOM'd files behave the same as those files, and does not alter the bug. #70 already proposes a fix for it.

The $had_utf8_bom to Content-Type wiring is the one piece of new logic with no automated coverage, because it lives in the procedural part of the script. The HTTP comparison above covers it, but nothing will catch a later regression.