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

[Var-Dumper] Fixed encoding issue #13440

Closed
wants to merge 1 commit into from

Conversation

arrilot
Copy link
Contributor

@arrilot arrilot commented Jan 18, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

Not ready yet. I need to change implementation first because this one is not working well enough

When I started using symfony/var-dumper in non-Symfony app i ran across the following encoding issue:

Applications files are in windows-1251 encoding,
Everything in browser output is OK except the var-dumper output which is broken:
before

After this fix, as soon as somewhere in bootstrap mb_detect_order is set as needed, we get the correct output:
after

Windows-1252 is explicitly added to the array of available encodings only for BC.

Please correct me I'm wrong

@fabpot
Copy link
Member

fabpot commented Jan 18, 2015

ping @nicolas-grekas

@nicolas-grekas
Copy link
Member

Thanks for reporting the issue @arrilot . The fix your propose is not reliable enough imho. I'm currently trying to find the best way to properly handle charset issues.

@nicolas-grekas
Copy link
Member

@arrilot can you please try #13441? Note that you may have to set php.ini default_charset to CP1251 or explicitly set a second $charset argument when instanciating HtmlDumper.

@arrilot
Copy link
Contributor Author

arrilot commented Jan 18, 2015

@nicolas-grekas Yeah, I was going to find another solution after i reaIized that mb_detect_encoding is worthless in this case.
Nice to see that you have already done all the work instead of me, thanks
I've tried your PR in my cp1251-app and everything seems ok. 👍

One more thing to note:
I haven't checked it, but i think that the "not-mbstring" part of AbstractDumper::utf8Encode() won't work well with encodings different from utf-8 and cp1252. I don't even know if it is worth supporting or not.

@fabpot
Copy link
Member

fabpot commented Jan 20, 2015

closing in favor of #13441

@fabpot fabpot closed this Jan 20, 2015
fabpot added a commit that referenced this pull request Feb 2, 2015
This PR was merged into the 2.6 branch.

Discussion
----------

[VarDumper] fix handling of non-UTF8 strings

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13440
| License       | MIT
| Doc PR        | -

Commits
-------

4205559 [VarDumper] fix handling of non-UTF8 strings
@arrilot arrilot deleted the var-dumper-encoding branch February 8, 2015 11:07
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.

None yet

3 participants