Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118524
MIME-Version: 1.0
References: <CANMwmbN8_Km+urXbNO7oWvNteYA2mTPNYyRgfaoypEK1Cw4F0A@mail.gmail.com>
 <CA+p1FaZDm3-h_LoOwv619wFyPg+2pAM8qC2La438sU_SPHZnCg@mail.gmail.com>
 <CABdc3WqsDc+80ATdX9+qstOkf+eUYCAQNPeV_FXcHiFumVQb8A@mail.gmail.com>
 <8D53AD5B-7CFC-4820-9EE4-FEB365D327A8@woofle.net> <CABdc3Wo7_qJOtebvA2GNGFUGFAut3tJOShXDBLHnRKqhrwVJ-g@mail.gmail.com>
 <d2f2c96c-3d45-49fc-ad0b-16e5a578891c@www.fastmail.com> <CAGBsUrcVmKRz=X8B1pVxavh1WE3aYTOMEbiWeYqTgky67atAdw@mail.gmail.com>
 <CA+p1FaZF8C12xn7q7r+1SGVHKca-+KWrv0WB1X-fSawnDz07GQ@mail.gmail.com>
 <CANLbj-rfHh9T84WJxcXW3nSj44mmXdDHT_XDM26WWKbiJKvJxw@mail.gmail.com> <afe236de-e35f-487a-93a6-250efc0f3f9d@www.fastmail.com>
In-Reply-To: <afe236de-e35f-487a-93a6-250efc0f3f9d@www.fastmail.com>
Date: Sat, 27 Aug 2022 01:31:21 +0200
Message-ID: <CANMwmbOZNBR0gog0V_UH7LDL9JPNpsEmhQKrEmBFhxu-nR6T_w@mail.gmail.com>
To: PHP Internals List <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] RFC json_validate() - status: Under Discussion
From: dev.juan.morales@gmail.com (juan carlos morales)

I now provide an update of the discussion.

The good, the bads, the open questions, etc.

All of this will go into the RFC also, as requested by the procedure
in https://wiki.php.net/rfc/howto

"Listen to the feedback, and try to answer/resolve all questions.
Update your RFC to document all the issues and discussions. Cover both
the positive and negative arguments. Put the RFC URL into all your
replies."

=3D=3D=3D UPDATES =3D=3D=3D

- Different users have tested the functionality and obtained the
promissed results. Also their feedback about it was positive.

- Most part of the community in the mailing list showed a positive
opinion about this RFC, and looks forward for its integration into
PHP.

- The ones that checked the code also agree that is small
implementation, easy to mantain, and at the same time provides a big
benefit for such small implementation.

- The community got involve very actively in the discussion of the RFC
and provided all kind of useful feedback, and also took the time to
test json_validate() by themselves.


=3D=3D=3D Bad reasons for json_validate() provided by the community =3D=3D=
=3D


- One member of the mailing list expressed that:

1) Incorporating such a small implementation that can be achieve with
userland code is not a good idea.

Quote: "If we keep the tendency to pollute already bloated standard
library with an army of small functions that could have not exists and
be replaced with normal PHP counterparts IMHO we'll end with
frustration from developers as I believe DX slowly falls down here."

2) json_validate() would only be useful for edge cases.

Quote: "A `json_decode()` is a substitute that IMO solves 99% of use cases.
If I'd follow your logic and accept every small addition that handles
1% of use cases, somebody will raise another RFC
for simplexml_validate_string or yaml_validate and the next PhpToken::valid=
ate.
All above can be valid if we trust that people normally validate 300MB
payloads to do nothing if they DON'T fail and there is nothing strange
about that."

3) The user also provided an implementation of a JSON parser written
in pure PHP. https://gist.github.com/brzuchal/37e888d9b13937891c3e05fead504=
2bc


=3D=3D=3D Good reasons for json_validate() provided by the community =3D=3D=
=3D


@@@ Use cases provided by some members, I quote:

- "Yes well-formed JSON from a trusted source tends to be small-ish.
But a validation function also needs to deal with non-well-formed
JSON, otherwise you would not need to validate it."

- "If with a new function (json_validate()) it becomes much easier to
defend against a Denial-of-Service attack for some parts of a JSON
API, then this can be a good addition just for security reasons."

- "fast / efficient validation of a common communication format
reduces the attack surface for Denial-of-Service attacks."

@@@ Memory usage

- During the test of json_validate() from some users, they were happy
about the memory usage that was zero in most cases (which is the main
benefit out this feature).
Someone also did a test with a very large string (75 MB) and only a
few bytes were needed as reported by him; also the same user reported
an execution speed improvement by a 20-25% over using json_decode().

@@@ Reasons not to depend on userland JSON parsers

Even possible to write an excellent JSON parser in PHP like one of the
members in the mailing list provided us, there are good reasons for
dont relying on userland solutions.

# 1 - User Tim D=C3=BCsterhus provided nice thoughts about this, in favor
to json_validate(), ... I quote him:

- "Writing a JSON parser is non-trivial as evidenced by:
https://github.com/nst/JSONTestSuite. I expect userland
implementations to be subtly buggy in edge cases. The JSON parser in
PHP 7.0+ is
certainly more battle-tested and in fact it appears to pass all of the
tests in the linked test suite."

- "Even if the userland implementation is written very carefully, it
might behave differently than the native implementation used by
json_decode() (e.g. because the latter is buggy for some reason or
because the correct behavior is undefined). This would imply that an
input string that was successfully validated by your userland parser
might ultimately fail to parse when passed to json_decode(). This is
exactly what you don't want to happen."


(Some other members including me, also share this opinion.)


# 2 - The JSON parser in PHP follows an special convention, marked in
the PHP documentation.

# 3 - We already have a JSON parser in PHP, that is used by
json_decode(); reusing the existing JSON Parser provides 100%
compatibility between the validation of a json-string, and the
decoding of it.

# 4 - The user Larry Gafield also provided good reason to integrate
this implementation into PHP. I quote him:

"The heuristic I use is that an API should be "reasonably complete" in
one location.  Having a half-assed API in C and the rest left to
inconsistent and redundant user-space implementations is a terrible
API; the same would apply for a user-space library that is half-assed
and leaves the rest to "someone else to write."  Naturally "reasonably
complete" is a somewhat squishy term, which is why it's a heuristic.

By that metric, yes, str_starts_with() and friends absolutely belonged
in core, because we already have a bunch of string functions and
str_starts_with() is by a wide margin the most common usage of
strpos().

By the same token, yes, json_validate() makes sense to include in the
main API, which means in C.  If there's a performance benefit to doing
so as well, that makes it an easy sell for me."



=3D=3D=3D Changes in the implementation =3D=3D=3D

@@@ THROW EXCEPTION ON ERROR

- The ability to throw an exception on error will be remove from the
implementation, as this was pointed not only by the users in the
mailing list, but also during code review. There are totally valid
arguments to remove this capability.



=3D=3D=3D Changes in the RFC =3D=3D=3D


- I removed 3 of the provided examples because did not adjust to the
RFC purpose.

- I still need to add the provided use cases provided by the community
into the RFC, where json_validate() will make a good impact.

- Updating the RFC requires time, that is why the mailing list will be
updated before the RFC itself.


=3D=3D=3D Clarification about the name =3D=3D=3D


- In the beginning I named the function "is_json()", but I was not
following the convention written in:

https://github.com/php/php-src/blob/master/CODING_STANDARDS.md#user-functio=
nsmethods-naming-conventions

That is why I adjusted the implementation with the name
"json_validate()" suggested not only during code review, but also by
some of the people in the internals-mailing list.


=3D=3D=3D Open issues/concerns =3D=3D=3D


@@@ Usage of JSON_INVALID_UTF8_IGNORE @@@

- I have my doubts now, because of this codes:

```
<php

var_dump(json_validate("\"a\xb0b\""), json_last_error_msg());
var_dump("------------");
var_dump(json_validate("\"a\xb0b\"", 512, JSON_INVALID_UTF8_IGNORE),
json_last_error_msg());
```

Results:

bool(false)
string(56) "Malformed UTF-8 characters, possibly incorrectly encoded"
string(12) "------------"
bool(true)
string(8) "No error"


Gives different results, but ...

```
<?php

var_dump(json_decode("{ \"a\xb0b\" : \"dummy\" }"), json_last_error_msg());
var_dump("------------");
var_dump(json_decode("{ \"a\xb0b\" : \"dummy\" }", 512,
JSON_INVALID_UTF8_IGNORE), json_last_error_msg());
```

Results in:
NULL
string(56) "Malformed UTF-8 characters, possibly incorrectly encoded"
string(12) "------------"
NULL
string(56) "Malformed UTF-8 characters, possibly incorrectly encoded"



So at a first look, seems the flag should also be remove, as with
json_validate() we say its valid, but then on json_decode() we get
NULL, even with the JSON_INVALID_UTF8_IGNORE provided; does not make
sense I believe.

What do you think? Is there a use case I am missing here? Is this flag
worth to have still?


@@@ Returning BOOLEAN or INT @@@

Option 1) Return boolean, and in case of error, rely on the functions
json_last_error() and json_last_error_msg() to get the proper error.

Option 2) Return integer number, following the already existing
constants for json errors, being the return values as:

JSON_ERROR_NONE
  No error has occurred

JSON_ERROR_DEPTH
  The maximum stack depth has been exceeded

JSON_ERROR_STATE_MISMATCH
  Invalid or malformed JSON

JSON_ERROR_CTRL_CHAR
  Control character error, possibly incorrectly encoded

JSON_ERROR_SYNTAX
  Syntax error

JSON_ERROR_UTF8
  Malformed UTF-8 characters, possibly incorrectly encoded

JSON_ERROR_RECURSION
  One or more recursive references in the value to be encoded

JSON_ERROR_INF_OR_NAN
  One or more NAN or INF values in the value to be encoded

JSON_ERROR_UNSUPPORTED_TYPE
  A value of a type that cannot be encoded was given

JSON_ERROR_INVALID_PROPERTY_NAME
  A property name that cannot be encoded was given

JSON_ERROR_UTF16
  Malformed UTF-16 characters, possibly incorrectly encoded


- From implementation point of view, is very easy to achieve both, we
just have to come to an agreement. I believe both approaches are
valid. I also got this observation during code review. Personally I
prefer BOOLEAN, because TRUE is more appeal to the understanding that
the json-string is valid ... than returning zero, which in PHP is a
falsy value by design. For me BOOLEAN will avoid confusions.


RFC: https://wiki.php.net/rfc/json_validate
Implementation: https://github.com/php/php-src/pull/9399