amp-wp
amp-wp copied to clipboard
ampproject
Include mbstring polyfill
Feature description
I originally opened this at https://github.com/google/web-stories-wp/issues/4898 but this might be useful as part of the AMP plugin itself.
https://github.com/ampproject/amp-wp/blob/708a65b2cb75e62a03f54bd3cbf1bd011de86cc4/lib/common/src/Dom/Document.php#L1072-L1076
The Document class uses mb_convert_encoding to fix the encoding if needed. That function is part of the mbstring extension which can be missing or outdated on certain servers.
For those cases, https://github.com/symfony/polyfill-mbstring/ might be useful.
Caveats:
- Might need to use PHP-Scoper to prefix the classes and prevent conflicts with other plugins using this polyfill
- Might need to use
mcaskill/composer-exclude-filesto prevent files from being loaded automatically and steer that ourselves.
Curious to hear thoughts on this.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation brief
QA testing instructions
Demo
Changelog entry
This is only necessary if a site is not using UTF-8 already, correct?
It looks like 95% of sites use UTF-8 already.
So probably very few sites would ever encounter an issue here?
Uhm, probably? Not sure how many sites have odd blog_charset configurations.
We had a couple of reports about mangled special chars in the past, where the encoding was off. But we'd also use mb_convert_encoding for other cases, such as https://github.com/google/web-stories-wp/pull/4859
A question was raised in php-css-parser about whether mbstring should be made a hard-requirement: https://github.com/sabberworm/PHP-CSS-Parser/issues/254. Nevertheless, polyfill-mbstring requires PHP 7.1+ so we can't use it anyway.
We use an older version (1.18) which still supports PHP 5.6+. So does 1.19 I think (at least from what I can tell after looking at https://github.com/symfony/polyfill-mbstring/commit/39d483bdf39be819deabf04ec872eb0b2410b531)