WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 5 months ago

#30613 new defect (bug)

Check the return value of wp_json_encode()

Reported by: pento Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.1
Component: General Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description

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.

Attachments (2)

30613.diff (17.9 KB) - added by pento 5 years ago.
30613.2.diff (20.2 KB) - added by TobiasBg 5 years ago.

Download all attachments as: .zip

Change History (12)

@pento
5 years ago

#1 @pento
5 years ago

  • Keywords has-patch added

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.

#2 @DrewAPicture
5 years ago

  • Keywords needs-docs added

#3 follow-up: @freeatlast
5 years ago

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

#4 in reply to: ↑ 3 @pento
5 years ago

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.

#5 follow-up: @TobiasBg
5 years ago

  • Milestone changed from Future Release to 4.2

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?

#6 @pento
5 years ago

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.

#7 in reply to: ↑ 5 ; follow-up: @DrewAPicture
5 years ago

  • Keywords needs-patch added; has-patch removed

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
5 years ago

#8 in reply to: ↑ 7 @TobiasBg
5 years ago

  • Keywords has-patch added; needs-docs needs-patch removed

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.

This ticket was mentioned in Slack in #core by drew. View the logs.


5 years ago

#10 @pento
5 years ago

  • Milestone changed from 4.2 to Future Release

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.

Note: See TracTickets for help on using tickets.