json_encode() behavior for incorrectly encoded strings

12 years ago by Nikita Popov — view source

unread

Hi internals!

We currently have a big mess concerning the behavior of json_encode()
with incorrectly encoded UTF-8 strings.

To summarize the situation:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:

The invalid string is replaced with "null", thus creating a partial
JSON serialization
json_encode() returns the partial serialization, not false as it should
A warning is thrown, but only if display_errors=off
json_last_error() returns JSON_ERROR_UTF8

PHP 5.3.14 behaves as follows:

json_encode() returns false, as it should
A warning is thrown always, even with display_errors=on
json_last_error() returns JSON_ERROR_UTF8
If the PHP_JSON_PARTIAL_OUTPUT_ON_ERROR option is specified, the
old behavior is restored

The reason for this situation is that a patch was applied for all
branches, and then reverted, because Stas didn't consider the change
towards always throwing a warning (even with display_errors=on)
appropriate without further discussion.

The backout though was done only on PHP 5.4 and master, but not on
the PHP 5.3 branch.

Thus we now have differing behavior between 5.3 and 5.4 (and PHP <= 5.3.13).

So, I'd like to ask whether the patch can be reapplied to 5.4 and
master or whether we should do something else to solve this problem.

Nikita

12 years ago by Pierre Joye — view source

unread

hi,

On Thu, Jun 21, 2012 at 12:21 AM, Nikita Popov
nikita.ppv@googlemail.com wrote:

We currently have a big mess concerning the behavior of json_encode()
with incorrectly encoded UTF-8 strings.

To summarize the situation:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:
* The invalid string is replaced with "null", thus creating a partial
JSON serialization
* json_encode() returns the partial serialization, not false as it should
* A warning is thrown, but only if display_errors=off
* json_last_error() returns JSON_ERROR_UTF8

PHP 5.3.14 behaves as follows:
* json_encode() returns false, as it should
* A warning is thrown always, even with display_errors=on
* json_last_error() returns JSON_ERROR_UTF8
* If the PHP_JSON_PARTIAL_OUTPUT_ON_ERROR option is specified, the
old behavior is restored

The reason for this situation is that a patch was applied for all
branches, and then reverted, because Stas didn't consider the change
towards always throwing a warning (even with display_errors=on)
appropriate without further discussion.

Well, not sure we have to discuss why such thing is bad.
display_errors must be respected.

Also I suspect that this change should not even be merged in 5.4 nor
5.3. At least not without real testing.

The backout though was done only on PHP 5.4 and master, but not on
the PHP 5.3 branch.

Thus we now have differing behavior between 5.3 and 5.4 (and PHP <= 5.3.13).

So, I'd like to ask whether the patch can be reapplied to 5.4 and
master or whether we should do something else to solve this problem.

5.3 should be brought in line with 5.4.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

12 years ago by Laruence — view source

unread

hi,

On Thu, Jun 21, 2012 at 12:21 AM, Nikita Popov
nikita.ppv@googlemail.com wrote:

We currently have a big mess concerning the behavior of json_encode()
with incorrectly encoded UTF-8 strings.

To summarize the situation:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:
* The invalid string is replaced with "null", thus creating a partial
JSON serialization
* json_encode() returns the partial serialization, not false as it should
* A warning is thrown, but only if display_errors=off
* json_last_error() returns JSON_ERROR_UTF8

PHP 5.3.14 behaves as follows:
* json_encode() returns false, as it should
* A warning is thrown always, even with display_errors=on
* json_last_error() returns JSON_ERROR_UTF8
* If the PHP_JSON_PARTIAL_OUTPUT_ON_ERROR option is specified, the
old behavior is restored

The reason for this situation is that a patch was applied for all
branches, and then reverted, because Stas didn't consider the change
towards always throwing a warning (even with display_errors=on)
appropriate without further discussion.

Well, not sure we have to discuss why such thing is bad.
display_errors must be respected.

Agree, and it shall be, no any exception.

thanks

Also I suspect that this change should not even be merged in 5.4 nor
5.3. At least not without real testing.

The backout though was done only on PHP 5.4 and master, but not on
the PHP 5.3 branch.

Thus we now have differing behavior between 5.3 and 5.4 (and PHP <= 5.3.13).

So, I'd like to ask whether the patch can be reapplied to 5.4 and
master or whether we should do something else to solve this problem.

5.3 should be brought in line with 5.4.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--

--
Laruence Xinchen Hui
http://www.laruence.com/

12 years ago by Nikita Popov — view source

unread

The reason for this situation is that a patch was applied for all
branches, and then reverted, because Stas didn't consider the change
towards always throwing a warning (even with display_errors=on)
appropriate without further discussion.

Well, not sure we have to discuss why such thing is bad.
display_errors must be respected.
This kind of very ugly error handling is currently used in several
places. It is meant to protect server administrators that are running
production servers with display_errors=On from leaking information
about encoding issues (which could be valuable to attackers). So the
error is only thrown if it isn't shown to the client but logged
instead. Personally I really don't like this, but I agree with Stas
that this is a security relevant issue and shouldn't be simply changed
without further discussion.

5.3 should be brought in line with 5.4.
So you think that the corrected code should be backed out from PHP 5.3
and the incorrect 5.4/master behavior be restored?

Thanks,
Nikita

12 years ago by Pierre Joye — view source

unread

hi,

On Thu, Jun 21, 2012 at 11:21 AM, Nikita Popov
nikita.ppv@googlemail.com wrote:

5.3 should be brought in line with 5.4.
So you think that the corrected code should be backed out from PHP 5.3
and the incorrect 5.4/master behavior be restored?

For the warning yes. For the rest, it should imo return false as well,
partial encoding or magic replacements make no sense here.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

12 years ago by Daniel Convissor — view source

unread

Hi Nikita:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:

A warning is thrown, but only if display_errors=off

So the warning is put into the error log? But only if display_errors is
off? And if display_errors is on, no warning is produced?

That's very counter-intuitive and un-PHP-like. Someone mentioned it's for
security reasons. By that logic, we should get rid of all error
messages because they all reveal some information that can be used by
attackers.

Thanks,

--Dan

--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335

12 years ago by Nikita Popov — view source

unread

On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Nikita:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:
* A warning is thrown, but only if display_errors=off

So the warning is put into the error log? But only if display_errors is
off? And if display_errors is on, no warning is produced?

That's very counter-intuitive and un-PHP-like. Someone mentioned it's for
security reasons. By that logic, we should get rid of all error
messages because they all reveal some information that can be used by
attackers.

Yes, I agree with this, but other people thing differently about it ;)

Personally I'd actually remove the warning completely with the return
false behavior. The return value makes clear that there was an error
and json_last_error() can be used to determine what exactly went
wrong. The warning seems redundant in this case and could actually
make the use more complicated.

Nikita

12 years ago by Laruence — view source

unread

On Thu, Jun 21, 2012 at 10:25 PM, Nikita Popov
nikita.ppv@googlemail.com wrote:

On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Nikita:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:
* A warning is thrown, but only if display_errors=off

So the warning is put into the error log? But only if display_errors is
off? And if display_errors is on, no warning is produced?

That's very counter-intuitive and un-PHP-like. Someone mentioned it's for
security reasons. By that logic, we should get rid of all error
messages because they all reveal some information that can be used by
attackers.

Yes, I agree with this, but other people thing differently about it ;)
I think I must missed some infos, I though you was talking that you
want to generate wanrings without regard of EG(display_errors).

thanks

Personally I'd actually remove the warning completely with the return
false behavior. The return value makes clear that there was an error
and json_last_error() can be used to determine what exactly went
wrong. The warning seems redundant in this case and could actually
make the use more complicated.

Nikita

--

--
Laruence Xinchen Hui
http://www.laruence.com/

12 years ago by Rasmus Lerdorf — view source

unread

On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Nikita:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:

A warning is thrown, but only if display_errors=off

So the warning is put into the error log? But only if display_errors is
off? And if display_errors is on, no warning is produced?

That's very counter-intuitive and un-PHP-like. Someone mentioned it's for
security reasons. By that logic, we should get rid of all error
messages because they all reveal some information that can be used by
attackers.

Yes, I agree with this, but other people thing differently about it ;)

Personally I'd actually remove the warning completely with the return
false behavior. The return value makes clear that there was an error
and json_last_error() can be used to determine what exactly went
wrong. The warning seems redundant in this case and could actually
make the use more complicated.

The problem with a warning here is that there is usually no way to
prevent it short of using @ or preceding all calls to htmlspecialchars()
with an iconv() call. A bad guy can simply send invalid UTF-8 bytes to a
web app and look for that warning to get a really good idea about the
server software being used. And yes, I know people should have
display_errors off in production, but this case is slightly different
because it is so universal. Other user-triggerable warnings are very
code-dependent and there is no universal trigger string you can send to
all PHP apps. Almost all PHP apps call htmlspecialchars() on user input
at some point.

The call here was whether to throw a warning at all and the idea was
that it was ok to do so as long as it didn't leak to the world for
improperly configured servers that had display_errors on. I would be
fine with removing the warning altogether since sanitizing the string is
an inherent characteristic of the function now and throwing warnings
about the normal operation of a function doesn't make much sense.

-Rasmus

12 years ago by Pierre Joye — view source

unread

hi Rasmus,

The problem with a warning here is that there is usually no way to
prevent it short of using @ or preceding all calls to htmlspecialchars()
with an iconv() call. A bad guy can simply send invalid UTF-8 bytes to a
web app and look for that warning to get a really good idea about the
server software being used. And yes, I know people should have
display_errors off in production, but this case is slightly different
because it is so universal. Other user-triggerable warnings are very
code-dependent and there is no universal trigger string you can send to
all PHP apps. Almost all PHP apps call htmlspecialchars() on user input
at some point.

I have no problem to raise a warning here, but it must respect display_error.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

12 years ago by Sherif Ramadan — view source

unread

The problem with a warning here is that there is usually no way to
prevent it short of using @ or preceding all calls to htmlspecialchars()
with an iconv() call. A bad guy can simply send invalid UTF-8 bytes to a
web app and look for that warning to get a really good idea about the
server software being used. And yes, I know people should have
display_errors off in production, but this case is slightly different
because it is so universal. Other user-triggerable warnings are very
code-dependent and there is no universal trigger string you can send to
all PHP apps. Almost all PHP apps call htmlspecialchars() on user input
at some point.

The call here was whether to throw a warning at all and the idea was
that it was ok to do so as long as it didn't leak to the world for
improperly configured servers that had display_errors on. I would be
fine with removing the warning altogether since sanitizing the string is
an inherent characteristic of the function now and throwing warnings
about the normal operation of a function doesn't make much sense.

-Rasmus

So in other words this patch I submitted should be fine?

https://github.com/php/php-src/pull/111

This particular patch doesn't raise any controversy since it doesn't
touch any of that other stuff and simply puts the defined behavior
back in place as it should have been.

12 years ago by Nikita Popov — view source

unread

I applied a few changes in
https://github.com/php/php-src/commit/84fe2cc890e49f40bac7c3ba74b3cfc6dc4cef2f
and https://github.com/php/php-src/commit/36fa17a5fae84ab332366a202f0a709279a2466a.

From the commit message:
<<<<<
json_encode() now returns bool(false) for all possible errors, throws the
respective warning and also sets the respective json_last_error() error
code. Three new error codes have been added:

JSON_ERROR_RECURSION
JSON_ERROR_INF_OR_NAN
JSON_ERROR_UNSUPPORTED_TYPE

To get a partial JSON output instead of bool(false) the option
JSON_PARTIAL_OUTPUT_ON_ERROR can be specified. In this case the invalid
segments will be replaced either by null (for recursion, unsupported type
and invalid JSON) or 0 (for Inf and NaN).

The warning for invalid UTF-8 stays intact and is thrown also with
display_errors = On. If this behavior is undesired this can be remedied
later.

So all branches now behave the same: bool(false) is returned, a
warning is thrown and an error code is set.

For now I chose to always throw the warning, but if something else
comes out in conclusion to this discussion I'll gladly change it to
some other behavior.

Nikita

On Sat, Jun 23, 2012 at 12:50 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:

So in other words this patch I submitted should be fine?

https://github.com/php/php-src/pull/111

This particular patch doesn't raise any controversy since it doesn't
touch any of that other stuff and simply puts the defined behavior
back in place as it should have been.

12 years ago by Stas Malyshev — view source

unread

Hi!

The warning for invalid UTF-8 stays intact and is thrown also with
display_errors = On. If this behavior is undesired this can be remedied
later.

Must we discuss it 1000 times anew? There was a reason why it didn't
throw warning when display_errors is on. The reason is that it is very
easy to feed the server configured with display_errors = on wrong JSON
and thus force it to reveal information, and this is largely beyond the
control of application writer.

But what I am worried about even more is getting back to that "commit
first, discuss later" mentality. Why we have again a commit in the
stable branch before we have a consensus decision what it should be doing?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Ferenc Kovacs — view source

unread

On Sat, Jun 23, 2012 at 9:53 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Hi!

The warning for invalid UTF-8 stays intact and is thrown also with
display_errors = On. If this behavior is undesired this can be remedied
later.

Must we discuss it 1000 times anew? There was a reason why it didn't
throw warning when display_errors is on. The reason is that it is very
easy to feed the server configured with display_errors = on wrong JSON
and thus force it to reveal information, and this is largely beyond the
control of application writer.

But what I am worried about even more is getting back to that "commit
first, discuss later" mentality. Why we have again a commit in the
stable branch before we have a consensus decision what it should be doing?

from the side, it doesn't seems to be a consensus about having this
exception not honoring the display_errors setting.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

12 years ago by Nikita Popov — view source

unread

Hi!

The warning for invalid UTF-8 stays intact and is thrown also with
display_errors = On. If this behavior is undesired this can be remedied
later.

Must we discuss it 1000 times anew? There was a reason why it didn't
throw warning when display_errors is on. The reason is that it is very
easy to feed the server configured with display_errors = on wrong JSON
and thus force it to reveal information, and this is largely beyond the
control of application writer.
Yes, I feel like we must discuss this a thousand times anew. There
clearly is no consensus about it. Just look at the responses in this
thread and in other discussions relating this issue. When people read
"throws a warning, but only if display_errors=Off" they automatically
replace the Off with an On in their mind. The behavior is simply that
unintuitive.
Also I'd like to point out that we already decided to not use this
behavior in other, similar situations. E.g. max_input_vars throws
regardless of display_errors (and yes, this was discussed when the
feature was introduced).

In any case, I don't care that much about how json_encode behaves,
so I'd be fine with either way. Would just be nice if someone could
tell me what that "way" should be. I didn't see any consensus in the
discussion.

But what I am worried about even more is getting back to that "commit
first, discuss later" mentality. Why we have again a commit in the
stable branch before we have a consensus decision what it should be doing?
I only wanted to sync the branches up. I didn't consider the warning
such an important aspect of this issue and thought that it would be
okay to simply adjust it in a separate commit.

Nikita

12 years ago by johannes@schlueters.de — view source

unread

Yes, I feel like we must discuss this a thousand times anew. There
clearly is no consensus about it. Just look at the responses in this
thread and in other discussions relating this issue. When people read
"throws a warning, but only if display_errors=Off" they automatically
replace the Off with an On in their mind. The behavior is simply that
unintuitive.

Maybe it is simply time to rethink the error handling.

The current system doesn't really allow to distinguish between
"debugging help" and true "error reporting". I don't have a good
proposal, but recently I often struggled whether to throw an E_NOTICE
from some code in order to help the user while debugging and found
myself adding extension-specific error handling routines ... this case
here seems to suffer from a related problem. Anybody up for an RFC?
(This might also include cleanup the usage of exceptions for internals,
and probably converting E_RECOVERABLE to exception)

johannes, just throwing some thoughts in the discussion

12 years ago by Stas Malyshev — view source

unread

Hi!

Maybe it is simply time to rethink the error handling.

This is true, current error handling has many deficiencies and is also
very expensive even when the errors are muted (we still produce the
whole error string, etc. even if we never display it). Unfortunately, BC
concerns prevailed last times we tried to fix it, but maybe somebody has
new ideas on that.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Rasmus Lerdorf — view source

unread

Hi!

Maybe it is simply time to rethink the error handling.

This is true, current error handling has many deficiencies and is also
very expensive even when the errors are muted (we still produce the
whole error string, etc. even if we never display it). Unfortunately, BC
concerns prevailed last times we tried to fix it, but maybe somebody has
new ideas on that.

Yes, I think we need to rethink warnings for the cases where the error
is part of the common use case. htmlspecialchars() getting passed
invalid Unicode chars, json_decode() getting non-json and json_encode()
getting non UTF-8 chars. These are all cases where if the function
generates an error, in order to avoid that error you have to do some
work beforehand to sanitize the input data which is a useless thing to
have to do since the functions themselves can do it.

For json_decode() we have json_last_error() to help with debugging.
Maybe a solution is to generalize that. We already mostly have it via
$php_errormsg/error_get_last().

And yes, I don't think a warning from json_encode() is useful because to
properly avoid it you would need an iconv() call before calling it and
that is pure overhead. Short of that your only recourse is to use @ and
that just isn't the right answer. In this I would suggest just having it
return false and have json_last_error() apply to json_encode() as well.

-Rasmus

12 years ago by Gustavo Lopes — view source

unread

On Sat, 23 Jun 2012 21:53:01 +0200, Stas Malyshev smalyshev@sugarcrm.com
wrote:

Hi!

The warning for invalid UTF-8 stays intact and is thrown also with
display_errors = On. If this behavior is undesired this can be remedied
later.

Must we discuss it 1000 times anew? There was a reason why it didn't
throw warning when display_errors is on. The reason is that it is very
easy to feed the server configured with display_errors = on wrong JSON
and thus force it to reveal information, and this is largely beyond the
control of application writer.

I find this argument unconvincing.

If json_encode() is not supposed to receive badly encoded data, then it's
the application writer's responsibility to make sure that the data is
correctly encoded. If, on the other hand, you think that's actually not a
error on the part of the caller to call json_encode() with badly encoded
data (i.e., just like htmlspecialchars() -- from which I removed the
warning removed in 5.4 --, json_encode() also accumulates the
responsibility validating some aspects of the input), then there's no
reason to have a warning at all.

In other words, if it's a precondition of json_encode() that the input is
correctly encoded (and the check is just a form of safe programming), then
there's indeed an error on the part of the application writer, if it
isn't, then there's no reason for any sort of warning.

(By the way, I'd prefer no warning at all, both for this and for inf/NAN.
We already can get the error from json_last_error() if we want to)

--
Gustavo Lopes

12 years ago by Nikita Popov — view source

unread

If json_encode() is not supposed to receive badly encoded data, then it's
the application writer's responsibility to make sure that the data is
correctly encoded. If, on the other hand, you think that's actually not a
error on the part of the caller to call json_encode() with badly encoded
data (i.e., just like htmlspecialchars() -- from which I removed the warning
removed in 5.4 --, json_encode() also accumulates the responsibility
validating some aspects of the input), then there's no reason to have a
warning at all.

In other words, if it's a precondition of json_encode() that the input is
correctly encoded (and the check is just a form of safe programming), then
there's indeed an error on the part of the application writer, if it isn't,
then there's no reason for any sort of warning.

(By the way, I'd prefer no warning at all, both for this and for inf/NAN. We
already can get the error from json_last_error() if we want to)

The main problem with not throwing a warning is that it makes
debugging the error quite hard. json_last_error() only gives you an
error code. So you have to use some script like the one used as an
example in php.net/json_last_error to figure out what the particular
error code actually means. This is obviously a lot more inconvenient
than simply seeing "Oh, this failed because I passed an Inf".

Nikita

12 years ago by Rasmus Lerdorf — view source

unread

The main problem with not throwing a warning is that it makes
debugging the error quite hard. json_last_error() only gives you an
error code. So you have to use some script like the one used as an
example in php.net/json_last_error to figure out what the particular
error code actually means. This is obviously a lot more inconvenient
than simply seeing "Oh, this failed because I passed an Inf".

So how about:

mixed json_last_error ( [bool $error_string = false] )

Returns the last error (if any) occurred during the last JSON
encoding/decoding. By default an integer constant from the table below
is returned. If $error_string is true, an array containing both the
error constant and a string representation of the error is returned.

12 years ago by Nikita Popov — view source

unread

So how about:

mixed json_last_error ( [bool $error_string = false] )

Returns the last error (if any) occurred during the last JSON
encoding/decoding. By default an integer constant from the table below
is returned. If $error_string is true, an array containing both the
error constant and a string representation of the error is returned.

Makes sense to me. So should I do this? Remove warnings + add string
parameter for json_last_error?

Nikita

12 years ago by Stas Malyshev — view source

unread

Hi!

Makes sense to me. So should I do this? Remove warnings + add string
parameter for json_last_error?

Looks fine.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Benjamin Eberlei — view source

unread

On Wed, Jun 27, 2012 at 1:41 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Hi!

Makes sense to me. So should I do this? Remove warnings + add string
parameter for json_last_error?

I think its weird that the parameter is called $error_string and setting it
to true means returning an array.

Why not in the spirit of others have a new function json_last_error_msg()
or something similar?

Looks fine.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Nikita Popov — view source

unread

On Wed, Jun 27, 2012 at 1:41 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:

Hi!

Makes sense to me. So should I do this? Remove warnings + add string
parameter for json_last_error?

I think its weird that the parameter is called $error_string and setting it
to true means returning an array.

Why not in the spirit of others have a new function json_last_error_msg() or
something similar?

I implemented it with json_last_error(true) returning just a string,
not an array. You can get the array using array(json_last_error(),
json_last_error(true)) if you really want. But usually you will only
need one of them as they both have different use cases. The
stringified version for debugging and the error code version for
detecting the error programmatically.

Nikita

12 years ago by Pierre Joye — view source

unread

hi,

Why not in the spirit of others have a new function json_last_error_msg() or
something similar?

I implemented it with json_last_error(true) returning just a string,
not an array. You can get the array using array(json_last_error(),
json_last_error(true)) if you really want. But usually you will only
need one of them as they both have different use cases. The
stringified version for debugging and the error code version for
detecting the error programmatically.

I do not think this signature is consistent with the other error
functions. I'd to go with the _msg or _string equivalent.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

12 years ago by Nikita Popov — view source

unread

hi,

Why not in the spirit of others have a new function json_last_error_msg() or
something similar?

I implemented it with json_last_error(true) returning just a string,
not an array. You can get the array using array(json_last_error(),
json_last_error(true)) if you really want. But usually you will only
need one of them as they both have different use cases. The
stringified version for debugging and the error code version for
detecting the error programmatically.

I do not think this signature is consistent with the other error
functions. I'd to go with the _msg or _string equivalent.

I looked at a few other error functions (of which we by the way we
have loads, which is a bad sign) and indeed it seems that they all
use a separate function to get the error message. Most use a
xyz_errno() + xyz_error() pair.

So it seems reasonable to instead provide the functionality via
json_last_error_msg().

Any objections?

Nikita

12 years ago by Nikita Popov — view source

unread

I looked at a few other error functions (of which we by the way we
have loads, which is a bad sign) and indeed it seems that they all
use a separate function to get the error message. Most use a
xyz_errno() + xyz_error() pair.

So it seems reasonable to instead provide the functionality via
json_last_error_msg().

I replaced json_last_error(true) with json_last_error_msg() now (in
https://github.com/php/php-src/commit/974324676b2436f159f42d9241c569f813471684).

I kind of get the feeling that the changes turned out much more
intrusive than originally thought. Maybe the best thing to do would be
to revert all changes on 5.3/5.4 (including the very original one that
landed only on 5.3) and only introduce the new behavior on master. Or
do you think that the changes are okay for the release branches?

Nikita

12 years ago by Pierre Joye — view source

unread

On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Nikita:

PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8
string is encountered:
* A warning is thrown, but only if display_errors=off

So the warning is put into the error log? But only if display_errors is
off? And if display_errors is on, no warning is produced?

damned :) I mean only display_errors is ON. obviously :)

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

json_encode() behavior for incorrectly encoded strings

Cheers,

Cheers,

Cheers,

Cheers,

But what I am worried about even more is getting back to that "commit first, discuss later" mentality. Why we have again a commit in the stable branch before we have a consensus decision what it should be doing?

Looks fine.

Looks fine.

Cheers,

But what I am worried about even more is getting back to that "commit
first, discuss later" mentality. Why we have again a commit in the
stable branch before we have a consensus decision what it should be doing?