Fixed bug #64874 (json_decode handles whitespace and case-sensitivity incorrectly) #436

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@hikari-no-yume
Contributor

hikari-no-yume commented Sep 10, 2013

This is a pull request for the PHP-5.4 branch but it should also be applied to 5.5 and master.

https://bugs.php.net/bug.php?id=64874

There is a small possible backwards-compatibility issue here: Previously, json_decode('tRUe'), and other capitalisation variants of true, false or null were accepted. However, the RFC states only the lowercase variants should be supported, and this pull request fixes that (as well as fixing the whitespace issue). Hence, once this bug is fixed, code that relied on incorrect capitalisations of these constants would break. That being said, I seriously doubt there is any that relies on it at all. json_encode does not output bad JSON like that, nor do any serialisers that I know of) and this issue will only occur when you have a top-level constant, i.e. "[tRue]" didn't work anyway, the only thing you could get away with was "tRue". Furthermore, the page on wiki.php.net states, albeit incorrectly, that it implements JSON (strictly speaking, it's a superse). Anyone who read the spec would not expect to rely on such incorrect capitalised constants.

@remicollet

This comment has been minimized.

Show comment
Hide comment
@remicollet

remicollet Sep 11, 2013

Contributor

Perhaps if would have be a better idea to split this request.

I think skipping space is a good idea and won't affect much current behavior (just more RFC compliant and less strict). So could be acceptable for stable version (5.4 or 5.5)

Forcing literal to lowercase only (even if RFC compliant) is a bit more problematic and should probably go to a new major version (5.6 ?).

P.S. Notice jsonc extension already ignores not significant spaces, and have the same problem with uppercase literal.

Contributor

remicollet commented Sep 11, 2013

Perhaps if would have be a better idea to split this request.

I think skipping space is a good idea and won't affect much current behavior (just more RFC compliant and less strict). So could be acceptable for stable version (5.4 or 5.5)

Forcing literal to lowercase only (even if RFC compliant) is a bit more problematic and should probably go to a new major version (5.6 ?).

P.S. Notice jsonc extension already ignores not significant spaces, and have the same problem with uppercase literal.

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume Sep 11, 2013

Contributor

Well, I'd be highly surprised if anyone is reliant on deserialising wrongly-capitalised JSON texts consisting only of a constant. "true", "false" and "null" but wrongly capitalised are the only json_decode inputs which could trip up. "[true]", for example, would not. There are no JSON serialisers which output that. In the case that, somehow, they are reliant on them, it would be trivial to do a case-insensitive comparison theirself. So I don't think this bug fix is likely to cause anyone anywhere a problem.

Contributor

hikari-no-yume commented Sep 11, 2013

Well, I'd be highly surprised if anyone is reliant on deserialising wrongly-capitalised JSON texts consisting only of a constant. "true", "false" and "null" but wrongly capitalised are the only json_decode inputs which could trip up. "[true]", for example, would not. There are no JSON serialisers which output that. In the case that, somehow, they are reliant on them, it would be trivial to do a case-insensitive comparison theirself. So I don't think this bug fix is likely to cause anyone anywhere a problem.

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume Sep 17, 2013

Contributor

Splitting request.

Contributor

hikari-no-yume commented Sep 17, 2013

Splitting request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment