Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue where minify() would corrupt some characters such as   in some environments #608

Merged
merged 2 commits into from
Aug 10, 2017

Conversation

benface
Copy link
Contributor

@benface benface commented Jul 21, 2017

Adding the u flag there fixes an issue in my environment (MAMP 4.1.1 with PHP 7.1.5) where minify() would corrupt the   character (it would get replaced by the replacement character).

…bsp;) in some environments

Adding the `u` flag there fixes an issue in my environment (MAMP 4.1.1 with PHP 7.1.5) where `minify()` would corrupt the ` ` character (it would get replaced by the replacement character).
@angrybrad
Copy link
Contributor

This also fixes it for 4-byte unicode characters, like 𠜎 𠜱 𠝹 𠱓.

@mrclay
Copy link
Owner

mrclay commented Jul 21, 2017

SGTM. Is this testable? Any thoughts @glensc?

@angrybrad
Copy link
Contributor

Would be difficult. Was only able to reproduce it using the latest version of MAMP. Assumed it had something to do with the version of PCRE installed (8.38), but tried outside of MAMP using that same PCRE version and it worked fine. Having a difficult time tracking down what the source is.

@benface
Copy link
Contributor Author

benface commented Aug 9, 2017

Just added some more u flags (in all the preg_ functions in HTML.php, basically) because I could still reproduce the issue with differently formatted HTML (still containing the   character).

@mrclay mrclay merged commit 70952d8 into mrclay:master Aug 10, 2017
@mrclay mrclay added this to the Minify 3.0.2 milestone Aug 10, 2017
@glensc
Copy link
Collaborator

glensc commented Sep 15, 2017

/u adds some kind of performance penalty, but i think it's worth the correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants