Failed test with 7.1.0RC6

[remicollet]

Detected in Fedora QA, since PHP update to 7.1.0RC6 https://apps.fedoraproject.org/koschei/package/php-pecl-pq?collection=f26

PASS cancel [tests/cancel001.phpt] 
TEST 17/53 [tests/conv001.phpt]
========DIFF========
002+ 
003+ Fatal error: Uncaught pq\Exception\DomainException: ERROR:  invalid input syntax for type json
004+ DETAIL:  The input string ended unexpectedly.
005+ CONTEXT:  JSON data, line 1: {"int":123,"obj":{"a":1,"b":2,"c":3 in /builddir/build/BUILD/php-pecl-pq-2.1.1/ZTS/tests/conv001.php:198
006+ Stack trace:
007+ #0 /builddir/build/BUILD/php-pecl-pq-2.1.1/ZTS/tests/conv001.php(198): pq\Connection->execParams('SELECT $1 as hs...', Array, Array)
008+ #1 {main}
009+   thrown in /builddir/build/BUILD/php-pecl-pq-2.1.1/ZTS/tests/conv001.php on line 198
002- array(1) {
...

[m6w6]

Hrm, confirmed; something's going wrong with PHP-7.1+ and JSON: https://travis-ci.org/m6w6/ext-pq/builds/175760021

[m6w6]

Duh, php_json_encode() returns PHP_JSON_ERROR_DEPTH because JSON_G(encode_max_depth) is 0.

[m6w6]

Obviously one has to set JSON_G(encode_max_depth) when using php_json_encode() internally...

[m6w6]

Gosh, and of yourse I don't have access to JSON_G if ext/json is loaded dynamically... Such an essential thing should have an API.

/cc @bukka

[m6w6]

Oh, nevermind, fix seems to work.

[remicollet]

indeed, fix works

Thanks

[remicollet]

BTW, I think this should be fixed in ext/json GINIT

json_globals->encode_max_depth = PHP_JSON_PARSER_DEFAULT_DEPTH;

Else, probably more ext will be affected...

/cc @bukka

[remicollet]

@krakjoe mays have some interest to be fixed before 7.1.0 GA

[remicollet]

And we perhaps even need a

#define PHP_JSON_ENCODER_DEFAULT_DEPTH 512

PHP_JSON_API int php_json_encode_ex(smart_str *buf, zval *val, int options, int depth);
PHP_JSON_API int php_json_encode(smart_str *buf, zval *val, int options) {
   php_json_encode_ex(buf, val, options, PHP_JSON_ENCODER_DEFAULT_DEPTH);
}

[jpauli]

:+1: For consistency, we need a

PHP_JSON_API int php_json_encode_ex(smart_str buf, zval val, int options, zend_long depth);

Why is php_json_decode() declared as public symbol, but NOT php_json_encode() ? Consistency is not good here ; that would need to be fixed I guess.

@krakjoe : I can take care of it, however, removing a public symbol may break some extensions ; but that would improve our overall consistency

[bukka]

@jpauli Please don't! This change was on purpose.

Please see https://github.com/php/php-src/pull/2173 for more info

[bukka]

The better solution would be expose json_encoder struct and json_encode_zval which could be aliased as php_json_encode_ex maybe. But I didn't want to do that initially as there might be some changes in the future. However if it's breaking exts right now, I will think about it.

[bukka]

I will actually take a look if I can fix this specific case in a simpler way first.

But the main point. Please don't expose anything yet!

[jpauli]

What I suggest is to add some consistency.

In an extension, if I use php_json_decode_ex() , I would blindly write php_json_encode_ex() thinking it would exist and have the same API. That's not the case actually, and lead to WTF moment, needs to look for the symbol to do the task , etc... That is not consistent , like many places in PHP source code. If we could add more logic in our internal API , I would be pleased.

[remicollet]

I think we should move to another place ;)

=> https://bugs.php.net/73526 (with simple patch proposal)

[bukka]

@m6w6 I'm actually wondering how this worked before 7.1. Does your code relay on the fact that json_encode user functions was called before you use it? Because from what I see that's the only way how the global would change that global that is used later.

The thing that changed in 7.1 is that json_encode doesn't set that which looks to me as the reason of the break. The reason why I used that global was actually not to break existing exts but I didn't realised that they might rely on setting from somewhere else...

Please can you confirm if that's the case?

[m6w6]

Looking through the code in master and 7.0 it seems like 7.0 did check the depth but didn't actually stop encoding and return from php_json_encode_array.

And while we're at it, it seems that I could override max_depth with PHP_JSON_PARTIAL_OUTPUT_ON_ERROR in 7.1/master.

[m6w6]

I think max_depth should be checked on function entry, not at the end.

[bukka]

Ah I see you don't actually care about errors in error_code global and always use returned JSON, right? Yeah that was actually a needed change to fix https://bugs.php.net/bug.php?id=70275 so you can't no longer rely on buffer to have any content when there is any error_code set. It was basically like always using PHP_JSON_PARTIAL_OUTPUT_ON_ERROR so I should maybe set it in php_json_encode as default to not break BC.

So probably best think to keep your fix and also have it for PHP 7.0 to not have error set.

I agree that it's not ideal and the internal API needs some changes to make it more usable for exts going forward. You are right about max_depth and I will fix that... ;)