Check the return value of wp_json_encode()

[pento]

It's possible for wp_json_encode() to return false, same as json_encode(). Everywhere in core, this will cause either invalid JS to be produced, or invalid JSON to be returned to an ajax call.

We need to sanity check the return value, and switch false for an appropriate response.

This ticket is branched from discussion in #28786.

[pento]

30613.diff​ is a first pass at this. For the most part, it just swaps false for an empty array, object or string, but I think we could add extra sanity checking on the JS side.

For example, the change in wp-admin/includes/template.php swaps false for "false", then the JS sanity checks the value before continuing.

[freeatlast]

I just updated to 4.1 and got this error Fatal error: Call to undefined function wp_json_encode() in /home/boystome/public_html/wp-includes/class.wp-scripts.php on line 186

Any help would be much appreciated

[pento]

Replying to freeatlast:

I just updated to 4.1 and got this error Fatal error: Call to undefined function wp_json_encode() in /home/boystome/public_html/wp-includes/class.wp-scripts.php on line 186

Any help would be much appreciated

Thanks for the report! I've created #30779 to track this bug.

[TobiasBg]

Let's do this in 4.2, to not unnecessarily stretch the time span between the introduction of wp_json_encode() and proper usage.

@DrewAPicture: What kind of docs do you have in mind here? Just something simple like // Sanity check. for each of the changes?

@pento: Any other reasons for using " instead of ' for the strings than making it more look like JSON?

[pento]

No reason. I may've been playing around with something else while I was writing the patch, and forgot to change everything back to ' strings.

[DrewAPicture]

Replying to TobiasBg:

@DrewAPicture: What kind of docs do you have in mind here? Just something simple like // Sanity check. for each of the changes?

Something like that. And it looks like there's an errant \ on the first line of src/wp-admin/includes/nav-menu.php in the patch.

Also, while I agree we should be doing these sanity checks, it's not exactly DRY to be explicitly checking false and assigning [] all over the place.

I realize that wp_json_encode() has parameter parity with json_encode() but it might be worth exploring adding a fourth parameter for the sanity check return. It would be a lot cleaner and could cover most of these cases.

[TobiasBg]

30613.2.diff​ is a refreshed version of @pento's original patch, with

• that errant \ removed,
• a short inline comment for each check,
• all occurrences of wp_encode_json() in Core covered.

Replying to DrewAPicture:
I'm not sure that I like the idea of a fourth parameter for wp_json_encode():

• The fourth parameter would force use to use the optional second and third parameters in all calls of the function, leading to ugly 0's and 512's popping up in the code (the default values of those parameters).
• We are currently using [], {}, false, and '' as the fallback values for the error case in these sanity checks. With the checks, we could also move to more sophisticated error handling later (like bailing out early or whatever).
• I don't think it's worth giving up readability of the checks by hiding the three lines in an optional function parameter.

[pento]

At the moment, the patch stops invalid JS from being generated, but that's still not great. Valid JS is a step up, but a long way from informing the user that something went wrong, or better yet - fixing the situation.

I'm not entirely sure what form that error handling would take - I suspect it'll be handled differently for each case.