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 UTF-8 handling for VueJs scanner. #242

Merged
merged 3 commits into from
Dec 2, 2019

Conversation

briedis
Copy link
Contributor

@briedis briedis commented Dec 2, 2019

DOMDocument does not handle UTF8 well without using htmlentities() or prepending XM encoding tag.

Added a fix and a test.

@MarekGogol
Copy link
Contributor

MarekGogol commented Dec 2, 2019

coincidence? #241 :)

@briedis
Copy link
Contributor Author

briedis commented Dec 2, 2019

@MarekGogol goddamnit. We just noticed this issue on Friday.

What do you think if this solution (prepending XML encoding tag)?

Calling htmlentities to convert characters seems a bit risky, not sure I trust PHP enough with encoding conversions (bad experience). Ran some benchmarks - xml tag approach is a tiny fraction faster too.

@MarekGogol
Copy link
Contributor

MarekGogol commented Dec 2, 2019

I think your solution is good enough. VueJs template may not consist of <xml> tag, as you sad...

To be honest, I am not 100% sure which one is better. I just brought solution from link below. But your solution might be faster, so we can apply yours...
https://stackoverflow.com/questions/8218230/php-domdocument-loadhtml-not-encoding-utf-8-correctly?answertab=active#tab-top

We should wait for the author of package, and we will see what next.

@briedis
Copy link
Contributor Author

briedis commented Dec 2, 2019

@MarekGogol Yeah, that is the same SO post I was looking at 😂

I'm really happy that people are using the vue scanner and even contributing. Was not expecting that! (I was the author for vue scanner code)

@briedis
Copy link
Contributor Author

briedis commented Dec 2, 2019

@oscarotero sorry to bother again, but without the fix package is unusable in production. 😬

@MarekGogol
Copy link
Contributor

MarekGogol commented Dec 2, 2019

@oscarotero sorry to bother again, but without the fix package is unusable in production. 😬

So thanks @briedis ! VueJs is one of my favorite scanner :). I just didn't realize for one year that all my existing translations from VueJs templates in client projects are crap :D (nobody told me). This week I was working on my own project, and I find it out 😄

image

@oscarotero
Copy link
Member

Ok. I have a package for that (https://github.com/oscarotero/html-parser/blob/master/src/Parser.php) but wouldn't like to add a dependency for this (maybe in v5, because we have a different package for each scanner).
I'm merging this.

@oscarotero oscarotero merged commit fabc8b9 into php-gettext:4.x Dec 2, 2019
@briedis briedis deleted the vuejs-utf8-fix branch December 2, 2019 11:31
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.

3 participants