search

zircote / Hal

A PHP implementation of HAL http://stateless.co/hal_specification.html
93 stars 6 forks source link

Add the JSON_NUMERIC_CHECK flag in __toJson #7

Closed dhrrgn closed 11 years ago

dhrrgn commented 11 years ago

This tells json_encode to convert all numeric strings to numbers.

baldurrensch commented 11 years ago

Looks good to me

baldurrensch commented 11 years ago

In general, it is considered good practice to not make PRs from the master branch.

dhrrgn commented 11 years ago

Sorry, just did it really quick. I can re-submit from a feature branch if you like.

baldurrensch commented 11 years ago

No, don't worry. This usually becomes more a problem with long outstanding PRs. :-)

hjr3 commented 11 years ago

I thought this was closed. Whoops!

Will there be a problem with JSON_NUMERIC_CHECK and large numbers?

zircote commented 11 years ago

In the event the value is greater than PHP_MAX_INT it will render that value in notational format:

php > echo PHP_INT_MAX , PHP_EOL;
9223372036854775807
php > $a = array('key' => "9223372036854775808");
php > echo json_encode($a, JSON_NUMERIC_CHECK), PHP_EOL;
{"key":9.2233720368548e+18}
php > $a = array('key' => "9223372036854775807");
php > echo json_encode($a, JSON_NUMERIC_CHECK), PHP_EOL;
{"key":9223372036854775807}
php >

I think this should be an active option the user must opt in to utilize and not default to this. I am open to others thoughts on this however.

edorfaus commented 11 years ago

I just wanted to pipe in with an agreement that this should not be required/default, as strings containing only digits are not necessarily intended as numbers - e.g. consider a base64-encoded chunk of data that just happens to only use digits in its base64 representation. Having it converted to scientific notation would destroy it.

Besides, if the user wants the JSON to use a number, they can simply make the value be a number instead of a string (e.g. using intval()).

hjr3 commented 11 years ago

@edorfaus i agree, which is why we cannot merge this PR as is.