wp_send_json silently fails on non UTF-8 strings

[pento]

When wp_send_json() is used to return a JSON-encoded lump of data, it silently fails if the data contains any non UTF-8 characters.

[pento]

attachment:28786.4.diff​

• Adds unit tests
• Tries to use mb_convert_encoding(), if available
• Removes some old code

[pento]

No objections? Cool. Lets commit it and see what happens.

[ocean90]

Related: #28807

[pento]

attachment:28786.5.diff​ falls back to treating the string as UTF-8 if it can't detect the encoding. This is the same behaviour as wp_check_invalid_utf8().

[pento]

attachment:28786.7.diff​

• Adds $options and $depth parameters, to match json_encode()
• A handful of performance improvements for _wp_json_sanity_check()
• Tidied up test_wp_json_encode()
• test_wp_json_encode_depth() added to make sure we obey $depth

[pento]

attachment:json_test.php​ does some performance testing. Results show that, for data that works fine with json_encode(), the overhead of wp_json_encode() is minimal. For extremely large data sets that need to be converted, performance can be pretty bad, but I don't think we're going to encounter much in the way of 6D arrays with 1,000,000 items in them. :-)

$ php json_test.php 
json_encode: 0.051064968109131
wp_json_encode overhead: 0.052061080932617
Best case wp_json_encode: 2.9125101566315
Worst case wp_json_encode: 15.276411056519

[DJPaul]

@pento's patch fixes the situation where an audio file with non UTF-8 in the meta description will cause the Media Library to not load. However, if that audio file is used in the playlist shortcode, it still breaks the player JS on the front-end. 28786.b1.diff patches the relevant part of media.php to use the new encode function.

[DrewAPicture]

#28807 was marked as a duplicate.

[DrewAPicture]

Closed #28807 as a duplicate, since both tickets serve to handle non-UTF-8 failures with json_encode(). We'll need to make sure that the plugin API response calls referenced in #28807 are handled here as well.

This will need lead dev support to get in for 4.0.

[pento]

attachment:28786.8.diff​

• Updates the patch to apply cleanly against trunk
• Adds the fix from attachment:28786.b1.diff​
• Fixes #28807

It's too late for this to go into 4.0, it can go in 4.1 early.

[pento]

28786.9.diff​ is a refresh to apply cleanly against trunk.

[pento]

#15827 was marked as a duplicate.

[pento]

28786.10.diff​ converts all json_encode() calls in WordPress to be wp_json_encode().

[mdawaffe]

I worry about mb_detect_encoding() but don't have any data or experience to validate or assuage that worry.

I know we're using this code on WordPress.com in places. In other places, we use code that explicitly converts from the blog_charset (no attempt at auto-detection) to UTF-8.

[pento]

28786.11.diff​ now also sanity checks array/object keys.

And because we all like things that go faster, unwinding unnecessary recursion gives us a bit of a boost when we need to sanity check the data.

$ php json_test.php 
json_encode: 0.055270910263062
wp_json_encode overhead: 0.058070182800293
Best case wp_json_encode: 0.98551392555237
Worst case wp_json_encode: 4.3197019100189

[azaozz]

Looking at 28786.11.diff, couple of nitpicks:

• _wp_json_convert_string() can be called many times. Instead of doing function_exists( 'mb_convert_encoding' ) every time, can probably set a static after the first check.
• A ​user note in the PHP manual mentions that mb_detect_encoding() should be run in strict mode (third arg set to true). Perhaps some "real life" testing for that is worth it.

Also wondering if we should use _wp_json_convert_string() when htmlspecialchars() fails (can probably rename it to something without 'json' and have an arg to convert to the 'blog_charset'). Still hear sometimes that wp_richedit_pre() and wp_htmledit_pre() may return an empty string especially for posts that were imported (so the charset is not the same as get_option( 'blog_charset' )).

[pento]

28786.12.diff​ uses a static var instead of function_exists(), and does strict mb_detect_encoding() checks.

I'm inclined to keep _wp_json_convert_string() as a function specifically for JSON. I'm working on a patch for WPDB that will provide more comprehensive character set conversion functionality.

[pento]

In 30055:

Add wp_json_encode(), a wrapper for json_encode() that ensures everything is converted to UTF-8.

Change all core calls from json_encode() to wp_json_encode().

Fixes #28786.

[JustinSainton]

Minor doc fix in 28786.13.diff

[pento]

In 30058:

Fix a PHPDoc typo for wp_json_encode().

Props JustinSainton.

See #28786.

[TobiasBg]

28786.docs.diff​ adds some documentation to the private functions, as per ​drew, fixes punctuation in some more docs, and changes else if to elseif, which seems to be preferred in the WP Coding Standards.

[markjaquith]

In 30075:

Define JSON_PRETTY_PRINT so it can be used with wp_json_encode()

• JSON_PRETTY_PRINT was introduced in PHP 5.4
• Now you can use it with lower PHP versions, without a notice

fixes #30139
see #28786

[DrewAPicture]

28786.docs.2.diff​ looks pretty good to me. Nice work @TobiasBg!

[markjaquith]

In 30078:

Docs and code standards cleanup for [30055] (wp_json_encode() & friends)

fixes #28786
props TobiasBg

[wonderboymusic]

Currently, wp_json_encode() returns false in its try/catch block - most of the functions that it is used in or passed to expect string.

Also problematic, if someone sets a JS var equal to the output, their code will explode - see ​https://github.com/WordPress/WordPress/commit/007ec52958742ed768c74f37e21e6db2b75aa32e#diff-aa3b133f32e8e18853fde3a700fb9a70R274

This may not totally matter, but:
​https://scrutinizer-ci.com/g/staylor/WordPress/inspections/c68e0913-d1dd-4dfd-bdcf-5b2dc68e0f3c/issues/files/wp-admin/includes/ajax-actions.php?status=new&orderField=path&order=asc

[pento]

wp_json_encode() returns false on failure, to match the behaviour of json_encode().

I'm not wild about changing wp_json_encode()'s behaviour (to return 0 or an empty string on failure, perhaps), though there's possibly value in having a function that always returns valid JSON, I think there being a clear "this thing failed to encode" return value is more valuable.

I think our better option is to fix the places that call wp_json_encode(), to ensure proper type checking.

[azaozz]

Replying to pento:

...(to return 0 or an empty string on failure, perhaps)

Right, also a "proper" JSON encoded value is 'false', '0', '""', etc. On the JS side JSON.parse() and jQuery.parseJSON() throw errors on empty string, int, etc.

We should probably handle the false returned from wp_json_encode() in wp_send_json_success() and wp_send_json_error().

[TobiasBg]

Replying to pento:

I think our better option is to fix the places that call wp_json_encode(), to ensure proper type checking.

I agree. This is much better than having wp_json_encode() not return false, which can be checked for to initiate error handling. If wp_json_encode() returns valid JSON (in terms of "empty" valid JSON) where json_encode() returns false, it would no longer be a drop-in replacement.
There would be no good way to find out if a wp_json_encode() return value like "" is the result of valid input (like an empty string), or the result of "wrong" input, that just could not be encoded.

[mdawaffe]

Replying to pento:

I think our better option is to fix the places that call wp_json_encode(), to ensure proper type checking.

I also agree :) json_encode() can also return false, so these places where a failing wp_json_encode() breaks things are already places where a failing json_encode() breaks things.

[MikeHansenMe]

see #30284

[pento]

In 30533:

Split the tests for wp_json_encode() into smaller chunks (let's all them "units"). Skip a couple of these tests when running on older versions of PHP that don't support the tested functionality.

See #28786.

[pento]

How we handle the return value from wp_json_encode() isn't a regression from how we handled the return value from json_encode().

Bumping to 4.2, I'll put a patch up in the next few days.

[helen]

We may be better closing this as fixed on 4.1 and opening a new ticket for the return value handling.

[pento]

I agree - we can continue this discussion in #30613.