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

Allow integers instead of floats for JSON. Fix #37. #714

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Aug 30, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets api-platform/api-platform#37
License MIT
Doc PR n/a

@dunglas
Copy link
Member Author

dunglas commented Aug 30, 2016

Tests failures related to willdurand/Negotiation#84, not to this patch. I'll send another PR to fix them tonight.

@dunglas dunglas merged commit 255873d into api-platform:master Aug 31, 2016
@dunglas dunglas deleted the fix_37 branch August 31, 2016 11:28
$builtinType = $type->getBuiltinType();
if (!call_user_func('is_'.$builtinType, $value)) {
if (false !== strpos($format, 'json') && Type::BUILTIN_TYPE_FLOAT === $builtinType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format check is unreliable and hackish...

Copy link
Member Author

@dunglas dunglas Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a better idea? It is simple and can be easily documented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is special handling for JSON, but what about other formats that we do not know about? Ideally the Normalizer shouldn't care about such things, right?

Copy link
Member Author

@dunglas dunglas Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other formats: it's why the method is protected. An alternative is to have a dedicated abstract JsonNormalizer but it looks overkill for now. Currently, we only support JSON based formats (the XML support is poor and buggy, and we have no other format supported).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same validation would occur without checking the format no? I don't see why this would be json-specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's a specificity of JSON to have a generic Number type. Other formats generally make the distinction between int and float.

fabpot added a commit to symfony/symfony that referenced this pull request Jan 6, 2017
…rializing JSON (dunglas)

This PR was squashed before being merged into the 3.1 branch (closes #21165).

Discussion
----------

[Serializer] int is valid when float is expected when deserializing JSON

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | api-platform/api-platform#37
| License       | MIT
| Doc PR        | n/a

JSON only has a Number type corresponding to both `int` and `float` PHP types.
PHP's `json_encode`, JavaScript's `JSON.stringify`, Go's `json.Marshal` as well as most other JSON encoders convert floating-point numbers like `12.0` to `12` (the decimal part is dropped when possible).
PHP's `json_decode` automatically converts Numbers without a decimal part to integers.

Actually, the Serializer rejects integers when a float is expected, this PR fixes this behavior when denormalizing JSON-based formats.

Port of api-platform/core#714.

/cc @gorghoa @Shine-neko

Commits
-------

4125455 [Serializer] int is valid when float is expected when deserializing JSON
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
Allow integers instead of floats for JSON. Fix api-platform#37.
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.

3 participants