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

Non-locale aware encoding of doubles, closes #1041 #1042

Merged
merged 3 commits into from
Feb 22, 2019

Conversation

Grundik
Copy link
Contributor

@Grundik Grundik commented Feb 21, 2019

Q A
Bug fix? yes
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1041
License MIT

@goetas
Copy link
Collaborator

goetas commented Feb 21, 2019

Thanks for this PR.

not sure if JSON encode is the best tool for the job here.

has anybody a better idea?

@Grundik can you crate a test case for this?

@Grundik
Copy link
Contributor Author

Grundik commented Feb 21, 2019

I've added tests. Strictly speaking, existing tests already failing on systems with inconvenient system locale. Its only a matter of default locale.

json_encode exports float value as string in machine-readable form without losing precision. Its the simplest way of accomplishment of such task. Another way could be switching to C locale, but that could cause severe performance degradation.

@goetas
Copy link
Collaborator

goetas commented Feb 22, 2019

What about this https://3v4l.org/WgoZh ? (does not require json)

@Grundik
Copy link
Contributor Author

Grundik commented Feb 22, 2019

Thats better, and does not requires integer hack. I've made changes to PR.

But: var_exports works good on floats only on PHP >= 7.0.2, so backporting this bugfix to older versions should be done via json_encode, if that would be done.

@goetas
Copy link
Collaborator

goetas commented Feb 22, 2019

This package requires php ^7.2 so it should not be a problem

@Grundik
Copy link
Contributor Author

Grundik commented Feb 22, 2019

We encountered this bug in 5.6, and we have so stick to this version for now, so I have backported this fix to 1.13.0 (https://github.com/Grundik/serializer/commits/bugfix-1041-1.13.0 if you're interested).

@goetas goetas added the bug label Feb 22, 2019
@goetas goetas merged commit 4826c96 into schmittjoh:master Feb 22, 2019
@goetas
Copy link
Collaborator

goetas commented Feb 22, 2019

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants