(wip) Posting a number when property type is string doesn't trigger an error #774
Conversation
| @@ -32,7 +33,7 @@ Feature: Create-Retrieve-Update-Delete | |||
| "dummy": null, | |||
| "dummyBoolean": null, | |||
| "dummyDate": "2015-03-01T10:00:00+00:00", | |||
| "dummyPrice": null, | |||
| "dummyPrice": 1.2, | |||
| @@ -62,7 +63,7 @@ Feature: Create-Retrieve-Update-Delete | |||
| "dummy": null, | |||
| "dummyBoolean": null, | |||
| "dummyDate": "2015-03-01T10:00:00+00:00", | |||
| "dummyPrice": null, | |||
| "dummyPrice": "1.2", | |||
There was a problem hiding this comment.
There it returns a string
| @@ -16,7 +16,8 @@ Feature: Create-Retrieve-Update-Delete | |||
| "value1", | |||
| "value2" | |||
| ] | |||
| } | |||
| }, | |||
| "dummyPrice": 1.2 | |||
There was a problem hiding this comment.
It should throw an error here because you pass a int and it wants a string. It's a bug.
|
Maybe ralted to symfony/symfony@62d28f9#diff-7a8fb8072d57f95ea6e37898b05895bc |
|
Yes so the only bug here is that we should send a string, but actually the number is accepted, so this test should fail. I'm leaving this open until I got time to fix it. |
soyuka
changed the title
soyuka
force-pushed
the
test-decimal-behavior
branch
from
05973e9 to
4580314
Compare
soyuka
force-pushed
the
test-decimal-behavior
branch
from
4580314 to
249eccc
Compare
| "dummyFloat": null, | ||
| "dummyPrice": null, | ||
| "dummyFloat": 1.2, | ||
| "dummyPrice": "1.2", |
There was a problem hiding this comment.
This fails and is a Number.
There was a problem hiding this comment.
This is no longer failing (see Travis build). So the bug is no longer present.
| } | ||
| }, | ||
| "dummyFloat": 1.2, | ||
| "dummyPrice": 1.2 |
There was a problem hiding this comment.
This is accepted. What should be the consistent behavior here:
- Only allowing strings when type is
Decimal? Allowing javascript numbers, but also serializingDecimaltypes as numbers?
There was a problem hiding this comment.
Note that I think the second solution would be cleaner, as that there are no difference between floats and decimals in json.
There was a problem hiding this comment.
No, decimal strings should always remain as strings even in JSON. Using float means possible loss of precision (the reason why we use decimal strings in the first place!)
So in other words, sending a floating-point value when the type is decimal is simply wrong. It is error-prone.
There was a problem hiding this comment.
Yeah, forgot that the precision loss is also due to php... Would you be able to tell me where I may fix this?
There was a problem hiding this comment.
Is this still a problem?
There was a problem hiding this comment.
The behaviour seems to be correct, since the test is failing (because dummyPrice cannot be float).
There was a problem hiding this comment.
wtf, had not updated dependencies sorry for wasting your time...
See PR comment.
Can someone tell me how we know it's a number, even though the
@varis typedstringhere?There might be a bug in the docs specifically with the@ORM(type=decimal)opposed tofloat(not proofed yet).