Helper functions for mbstring.func_overload

[dd32]

Similar to #25061, WP_HTTP_Streams doesn't account for mbstring.func_overload, and since it's also going to be an issue with WP_Filesystem in #25237 I've gone searching for a simpler solution.

Attached is a patch which introduces a helper function to set/reset the mbstring internal encoding, limited testing performed, but it appears to work as expected.
It's possible to call it multiple times (nested) as long as there are the same number of reset calls made.

An alternative solution, would be to set the mbstring internal encoding globally for WordPress, forcing it to our preferred character encoding, something that although I love the idea of at first, it may have significant impacts for users who actually rely upon it.

Commits where we've implemented workarounds: [25051] [17592]
It also appears the ​ID3 library doesn't like mbstring.func_overload, in addition to ​pomo having a ​set of workarounds for it.

[dd32]

Attachment attachment:25259.diff​​ added

This patch also moves the mbstring handling from the cURL callbacks, to the WP_HTTP::request() method, this allows it to apply the fix to all string calls used through out the HTTP class while not having to sprinkle calls around the transports liberally.

[SergeyBiryukov]

As noted ticket:25061:12, function_exists( 'ini_get' ) seems unnecessary, as we don't check for that anywhere else in core, other than in ​_load_image_to_edit_path() ([20806]), which I guess was a copy/paste from WP_Http_Streams::test(), from where it was removed in [25224].

[dd32]

function_exists( 'ini_get' ) seems unnecessary

I think we've phased it out over time, It was common as a disable_function on bad PHP4 hosts, and as a result was included often in checks previously.

We can probably remove the other remaining case in ​_load_image_to_edit_path() which got it's check from the now-defunct PHP4 WP_HTTP_Fopen transport.

[dd32]

In 25346:

Add a set of helpers to turn the behaviour of mbstring.func_overload off when needed. Fixes #25259

[dd32]

In 25347:

Switch unzip_file() over to using the mbstring.func_override helper functions. See #25259

[dd32]

In 25348:

Switch WP_HTTP over to using the mbstring.func_overload helper functions. This change moves the check from within the Streaming-handling function to wrap the individual request, this fixes it for both cURL and Streams and any future changes to the transports which use strlen() on binary data. See #25259 See #16057

[dd32]

In 25349:

Make use of the mbstring.func_overload helper functions in WP_Filesystem so byte lengths are properly determined. See #25259 Fixes #25237

[nacin]

I meant to comment here; it appears I didn't.

Would it make sense to have a function specifically for strlen() that does the switch, the measurement, and the restoration? This seems to be the most common use case in core. mb_substr() is used more, and mb_strtolower() is also used a few times. It just might make things a bit more clear what is going on. I'm fine with the helpers, but it isn't entirely obvious what is going on in between the two points. Some of the gaps are 10 to 25 lines, with other function calls in between — and potentially filters? We should aim to keep this as atomic as possible.

Reopening for review, especially given that this could have a side effect on other functions or plugins without thorough review.

[dd32]

Would it make sense to have a function specifically for strlen() that does the switch, the measurement, and the restoration?

I considered that, but, certain parts of the code use a bunch of other things, and I'd rather be consistent rather than using 2 different work arounds.

class-ftp.php & class-pclzip.php for example needs ereg_replace/strlen/substr/strstr/stristr and a few others, as they're all operating on binary data, so a full switch needs to take place to remain compatible there.

For things like WP_HTTP, strlen and substr are only needed (at present), and aside from the ftpsocketsWP_Filesystem transport, the rest only really need strlen at present.

For those cases, we could do a compat function that looks something like:

function wp_strlen( $string ) {
  return function_exists( 'mb_strlen' ) ? mb_strlen( $string, 'ascii' ) : strlen( $string );
}
function wp_substr( $string, $start, $length ) {
  return function_exists( 'mb_substr' ) ? mb_substr( $string, $start, $length, 'ascii' ) : substr( $string, $start, $length );
}

With the commit as-is, On most hosts these helpers will do absolutely nothing, it'll just be an extra function call, with a if ( static var is false ) return, on the rest of the hosts who have this stupid setting enabled, it'll use the fix we've been using for a while elsewhere..

[SergeyBiryukov]

Replying to dd32:

For those cases, we could do a compat function that looks something like:

FWIW, we already have a compat version of mb_substr(): tags/3.6.1/wp-includes/compat.php#L16​.

[dd32]

Replying to SergeyBiryukov:

Replying to dd32:

For those cases, we could do a compat function that looks something like:

FWIW, we already have a compat version of mb_substr(): tags/3.6.1/wp-includes/compat.php#L16​.

I guess the function_exists() check would have to either check for a different mbstring function, or, now that i look at it, simply calling it with an ascii encoding would always work (as if mbstring isn't loaded, it'll use substr(), otherwise, it'll use mb_substr with ascii)

[pbaylies]

On the topic of function_exists() checks, mbstring is a non-default extension; so you'll want to be doing function_exists() checks for mbstring_binary_safe_encoding() and reset_mbstring_encoding() here as well (this is currently broken in trunk for me as of [25348]).

[SergeyBiryukov]

Replying to pbaylies:

On the topic of function_exists() checks, mbstring is a non-default extension; so you'll want to be doing function_exists() checks for mbstring_binary_safe_encoding() and reset_mbstring_encoding() here as well (this is currently broken in trunk for me as of [25348]).

There is a function_exists( 'mb_internal_encoding' ) check in mbstring_binary_safe_encoding(): trunk/src/wp-includes/functions.php?rev=25400#L4178​.

[pbaylies]

SergeyBiryukov,

I'm not sure why those functions weren't defined for me previously then, but it all appears to be fine now; thanks!