MJ Convento

/

JSON_THROW_ON_ERROR earned its keep

1,198 words, about 5 min read

For nineteen days our webhook endpoint answered 200 to a courier partner and did nothing with the body. No exception, no error log, no alert. The handler decoded the JSON, read the events key, found nothing there, decided there was nothing to process, and replied politely.

The payload was fine most of the time. Roughly one request in three hundred arrived truncated, and json_decode() reports that by returning null, which is also exactly what it returns when it successfully decodes the JSON document null. We have been on 7.3 since March and on 7.3.7 since it shipped on 4 July. The flag that would have caught this has been in the language since 7.3.0 landed on 6 December 2018, and I had skipped it because it looked like a nicety.

The check nobody writes twice

The JSON_THROW_ON_ERROR RFC states the problem in its first paragraph better than I can: json_decode() returns null on error, null is also a possible valid result, and the only way to tell is to ask json_last_error(), which is global error state left over from the last call. Neither function throws a warning by default. Nothing in the language pushes you toward checking.

So I grepped. Sixty-one json_decode() call sites in the app, nine of them followed by a json_last_error() !== JSON_ERROR_NONE check. Six of those nine were in one file, written by one person, in one afternoon in 2018. And one of the six was wrong in a way I found genuinely funny: it logged the decoded value through a helper that called json_encode() first, and a successful encode resets the global error state, so by the time the check ran, json_last_error() said JSON_ERROR_NONE. The check was there. It could not fail.

That is the real argument for the flag. Not that the manual approach is verbose, but that global error state on a function you call eight times per request is state you will read at the wrong moment.

One flag, one exception

<?php
// src/Webhook/PartnerEventsController.php

// before: null means malformed, and null also means null
$payload = json_decode($request->getContent(), true);
if (!isset($payload['events'])) {
    return new JsonResponse(['accepted' => 0]);
}

// the body we were handing it, once every few hundred calls:
// {"shipment":"AB8812740","events":[{"code":"OUT_FOR_DELIVERY","at":"2019-07-0

// after
try {
    $payload = json_decode($request->getContent(), true, 512, JSON_THROW_ON_ERROR);
} catch (JsonException $e) {
    $this->logger->warning('partner payload rejected', [
        'code'  => $e->getCode(),     // 4, JSON_ERROR_SYNTAX
        'error' => $e->getMessage(),  // Syntax error
        'bytes' => strlen($request->getContent()),
    ]);

    return new JsonResponse(['error' => 'malformed json'], 400);
}

The exception carries the message and code that json_last_error_msg() and json_last_error() would have given you, so the code is one of the documented error constants and the message for a truncated body is the string “Syntax error”. JsonException extends Exception, so a catch (Exception $e) further up will still catch it, which is worth knowing before you assume it escapes to your error handler.

Two details from the RFC that matter in practice. The flag leaves the global error state untouched, so mixing flagged and unflagged calls does not confuse json_last_error() any further than it already is. And JSON_PARTIAL_OUTPUT_ON_ERROR takes precedence over it, so a wrapper that always passes the partial-output flag silently disables throwing. We had no such wrapper. I checked, because I no longer trust myself on this.

In the first week after deploying, 412 rejections. All from one partner, all truncated within two bytes of the same length, which pointed at something in front of their application rather than at their application. That conversation took four days and would not have started at all without the 400s.

The heredoc change that broke a template

The flexible heredoc syntax is the 7.3 change I would call genuinely dangerous in an old codebase, and the UPGRADING file says why: doc strings containing the ending label inside their body may now cause a syntax error or change meaning. Ours did.

<?php
// src/Notification/Template/DeliveryNotice.php
$body = <<<NOTE
Hi {$name},

Booking {$ref} is out for delivery today.
NOTE: all rates shown are inclusive of VAT.
NOTE;

// 7.2: one string, four lines. The VAT line is text, because a closing marker
//      had to be followed by nothing but an optional semicolon and a newline.
// 7.3: the string ends on the VAT line, and ": all rates shown are inclusive
//      of VAT." is now code. Compile error, caught by CI, not by a human.

// My first fix was to indent the closing marker, the new way. One body line
// in a neighbouring heredoc was still flush left, so I got this instead:
// Parse error: Invalid body indentation level (expecting an indentation level of at least 5)

// The fix the UPGRADING file actually recommends: pick a label that does not
// occur at the start of any line in the body. We now use NOTICE_BODY.

Four files in our codebase used a label that appeared inside the body. Two were plain text email templates, one was a help block in an admin page, one was dead code. If you are still on 7.2, grep for your heredoc labels before you upgrade rather than after.

The three I actually use every week

is_countable() replaces the is_array($x) || $x instanceof Countable pair that 7.2 forced on everyone when counting an uncountable value became a warning. array_key_first() and array_key_last() return the first and last key without reset() or end() moving the internal pointer, and return null on an empty array instead of false. The value variants in the same RFC were not implemented, so array_value_first() does not exist; I have typed it twice. And trailing commas are now allowed in function and method calls, which makes multi-line calls diff like arrays do.

None of those three are worth an upgrade on their own. The flag was. There are still 52 unflagged json_decode() calls in the app and I am not adding a wrapper class for them, because a wrapper is a place where someone will later pass JSON_PARTIAL_OUTPUT_ON_ERROR. They get the flag as we touch them, and a CI grep counts them so the number cannot go up.

Sources