Page MenuHomePhabricator

JsonContent incorrectly reports invalid JSON is valid, only on HHVM
Closed, DeclinedPublic

Description

I've been debugging a test that only fails in CI and found that it's due to an HHVM glitch. JsonContent calls FormatJson::parse, which calls the native json_decode and checks for errors. Here's my test script and output on different environments,

$badJson = '"foo": b }';

$decoded = json_decode( $badJson, true );
$error = json_last_error();

print( var_export( $decoded, true ) . "\n" );
print( var_export( $error , true ) . "\n" );

On Zend PHP 7.1.16, the output is:

NULL
4

With HHVM 3.18.6-dev, I get:

'foo'
0

Note that both evaluation engines correctly throw errors for a test string 'foo: b }', so I'm assuming the edge case has to do with a valid JSON item coming at the beginning of the string, and HHVM ignoring the "leftovers".

Details

Related Gerrit Patches:

Event Timeline

awight created this task.Oct 19 2018, 10:52 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 19 2018, 10:52 PM

There are clues in T128029, T74778, T128029. I've worked around for us, and hopefully the json-schema check gives us strict enough validation that we won't be allowing JSON that fails when app nodes are using Zend PHP.

Gross, another edge case is the empty string, both parsers return NULL but HHVM returns no error code so FormatJson::parse emits Status::newGood( null ).

awight removed a subscriber: awight.Mar 21 2019, 4:04 PM
Krinkle closed this task as Declined.Oct 3 2019, 2:32 AM
Krinkle added a subscriber: Krinkle.

Declining per T192166.

Change 547804 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/Jade@master] Restore commented-out test now that HHVM isn't supported

https://gerrit.wikimedia.org/r/547804

Change 547804 merged by jenkins-bot:
[mediawiki/extensions/Jade@master] Restore commented-out test now that HHVM isn't supported

https://gerrit.wikimedia.org/r/547804