Make WordPress Core

Opened 10 years ago

Last modified 5 years ago

#29717 new enhancement

wp_check_invalid_utf8 - pcre tricks and failsafes, +mb_convert_encoding, iconv fix, performance

Reported by: askapache's profile askapache Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch dev-feedback needs-refresh needs-unit-tests
Focuses: performance Cc:

Description

Used in core in these 4 functions.

  • esc_attr()
  • esc_js()
  • esc_html()
  • sanitize_text_field()

It's the first function to execute for all 4, and especially for sanitize_text_field it gets called quite a bit and is pretty important.

It's purpose is to check a string for invalid utf. It utilizes preg_match with the '/u' modifier to parse both the pattern and subject for utf. PCRE automatically checks both the pattern and subject for invalid utf, upon which it will exit with an error code/constant.

The changes here: Normally pcre is compiled with utf support. It can also be compiled to disallow utf support, and it can be compiled without utf support. If utf is compiled and enabled the '/u' modifier for preg_match is available which turns on the automatic utf validation.

For older dists or those with utf support turned off at compile, there is a trick to enable the same functionality as the '/u' provides.

http://www.pcre.org/pcre.txt
In order process UTF-8 strings, you must build PCRE to include UTF-8
support in the code, and, in addition, you must call pcre_compile()
with the PCRE_UTF8 option flag, or the pattern must start with the
sequence (*UTF8). When either of these is the case, both the pattern
and any subject strings that are matched against it are treated as
UTF-8 strings instead of strings of 1-byte characters.

So the first change to this function was to allow a fallback to that pattern option trick in case '/u' wasnt supported.

  1. @preg_match( '//u', '' ) !== false
  2. @preg_match( '/(*UTF8)/', '' ) !== false
  3. Fallback to a regex that doesn't require UTF support, instead of using pcre utf validation it searches for it

I also wanted it to have better performance, especially due to its use in those 4 core functions I use often. I benchmarked it pretty thoroughly to try and gain more speed. This patch is about 10-20% faster.

Many gains were from refactoring the logic and control structures, chaining within if statements using bools, and utilizing the static variables to the fullest. This is especially crucial since this function gets called repeatedly. I also gained some cycles by replacing an in_array() check with a stripos.

One of the bigger gains came from replacing the strlen( $string ) == 0 that ran on every run with. Since the $string variable was already casted to a string, that should always work and keep things a little cheaper.

$string = (string) $string;

// if string length is 0 (faster than strlen) return empty
if ( ! isset( $string[0] ) )
	return '';

The final change was to the 2nd parameters $strip, which if true is supposed to strip the invalid utf out of the string and return the valid. In core nowhere is that parameter being used (yet), which explains the deprecated looking iconv. Also added a fallback to use mb_convert_encoding in case iconv is missing.

// try to use iconv if exists
if ( function_exists( 'iconv' ) )
	return @iconv( 'utf-8', 'utf-8//ignore', $string );

// otherwise try to use mb_convert_encoding, setting the substitue_character to none to mimic strip
if ( function_exists( 'mb_convert_encoding' ) ) {
	@ini_set( 'mbstring.substitute_character', 'none' );
	return @mb_convert_encoding( $string, 'utf-8', 'utf-8' );
}

Here are some of the test strings I used, I also used the utf-8-test file at http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt. I did testing on 4.0 using php 5.6, 5.4, 5.3, and 5.4. I verified the output and the strip feature as well. For all tests I had php error_reporting set to the max:

ini_set( 'error_reporting', 2147483647 );
$valid_utf = array(
	"\xc3\xb1", // 'Valid 2 Octet Sequence' 
	"\xe2\x82\xa1", // 'Valid 3 Octet Sequence' => 
	"\xf0\x90\x8c\xbc", // 'Valid 4 Octet Sequence' => 
	"\xf8\xa1\xa1\xa1\xa1", //'Valid 5 Octet Sequence (but not Unicode!)' => 
	"\xfc\xa1\xa1\xa1\xa1\xa1", //'Valid 6 Octet Sequence (but not Unicode!)' => 
	"Iñtërnâtiônàlizætiøn\xf0\x90\x8c\xbcIñtërnâtiônàlizætiøn", // valid four octet id
	'Iñtërnâtiônàlizætiøn', // valid UTF-8 string
	"\xc3\xb1", // valid two octet id
	"Iñtërnâtiônàlizætiøn\xe2\x82\xa1Iñtërnâtiônàlizætiøn", // valid three octet id
);

$invalid_utf = array(
	"\xc3\x28", //'Invalid 2 Octet Sequence' => 
	"\xa0\xa1", //'Invalid Sequence Identifier' => 
	"\xe2\x28\xa1", //'Invalid 3 Octet Sequence (in 2nd Octet)' => 
	"\xe2\x82\x28", //'Invalid 3 Octet Sequence (in 3rd Octet)' => 
	"\xf0\x28\x8c\xbc", //'Invalid 4 Octet Sequence (in 2nd Octet)' => 
   "\xf0\x90\x28\xbc", // 'Invalid 4 Octet Sequence (in 3rd Octet)' => 
	"\xf0\x28\x8c\x28", //'Invalid 4 Octet Sequence (in 4th Octet)' => 
	chr(0xE3) . chr(0x80) . chr(0x22), // Invalid malformed because 0x22 is not a valid second trailing byte following the leading byte 0xE3. http://www.unicode.org/reports/tr36/
	chr(0xF8) . chr(0x80) . chr(0x80) . chr(0x80) . chr(0x80), // Invalid UTF-8, overlong 5 byte encoding.
	chr(0xD0) . chr(0x01), // High code-point without trailing characters.
	chr(0xC0) . chr(0x80), // Overlong encoding of code point 0
	chr(0xF8) . chr(0x80) . chr(0x80) . chr(0x80) . chr(0x80), // Overlong encoding of 5 byte encoding
	chr(0xFC) . chr(0x80) . chr(0x80) . chr(0x80) . chr(0x80) . chr(0x80),  // Overlong encoding of 6 byte encoding      
	chr(0xD0) . chr(0x01), // High code-point without trailing characters
	"Iñtërnâtiôn\xe9àlizætiøn", // invalid UTF-8 string
	"Iñtërnâtiônàlizætiøn\xfc\xa1\xa1\xa1\xa1\xa1Iñtërnâtiônàlizætiøn", // invalid six octet sequence
	"Iñtërnâtiônàlizætiøn\xf0\x28\x8c\xbcIñtërnâtiônàlizætiøn", // invalid four octet sequence
	"Iñtërnâtiônàlizætiøn \xc3\x28 Iñtërnâtiônàlizætiøn", // invalid two octet sequence
	"this is an invalid char '\xe9' here", // invalid ASCII string
	"Iñtërnâtiônàlizætiøn\xa0\xa1Iñtërnâtiônàlizætiøn", // invalid id between two and three
	"Iñtërnâtiônàlizætiøn\xf8\xa1\xa1\xa1\xa1Iñtërnâtiônàlizætiøn", //  invalid five octet sequence
	"Iñtërnâtiônàlizætiøn\xe2\x82\x28Iñtërnâtiônàlizætiøn", // invalid three octet sequence third
	"Iñtërnâtiônàlizætiøn\xe2\x28\xa1Iñtërnâtiônàlizætiøn", // invalid three octet sequence second
);

Notes and more info:


	In  order  process  UTF-8 strings, you must build PCRE to include UTF-8
	support in the code, and, in addition,  you  must  call  pcre_compile()
	with  the  PCRE_UTF8  option  flag,  or the pattern must start with the
	sequence (*UTF8). When either of these is the case,  both  the  pattern
	and  any  subject  strings  that  are matched against it are treated as
	UTF-8 strings instead of strings of 1-byte characters.



   UTF-8 was devised in September 1992 by Ken Thompson, guided by design
   criteria specified by Rob Pike, with the objective of defining a UCS
   transformation format usable in the Plan9 operating system in a non-
   disruptive manner.
   
   
    Char. number range  |        UTF-8 octet sequence
      (hexadecimal)    |              (binary)
   --------------------+---------------------------------------------
   0000 0000-0000 007F | 0xxxxxxx
   0000 0080-0000 07FF | 110xxxxx 10xxxxxx
   0000 0800-0000 FFFF | 1110xxxx 10xxxxxx 10xxxxxx
   0001 0000-0010 FFFF | 11110xxx 10xxxxxx 10xxxxxx 10xxxxxx  


   A UTF-8 string is a sequence of octets representing a sequence of UCS
   characters.  An octet sequence is valid UTF-8 only if it matches the
   following syntax, which is derived from the rules for encoding UTF-8
   and is expressed in the ABNF of [RFC2234].

   UTF8-octets = *( UTF8-char )
   UTF8-char   = UTF8-1 / UTF8-2 / UTF8-3 / UTF8-4
   UTF8-1      = %x00-7F
   UTF8-2      = %xC2-DF UTF8-tail
   UTF8-3      = %xE0 %xA0-BF UTF8-tail / %xE1-EC 2( UTF8-tail ) /
                 %xED %x80-9F UTF8-tail / %xEE-EF 2( UTF8-tail )
   UTF8-4      = %xF0 %x90-BF 2( UTF8-tail ) / %xF1-F3 3( UTF8-tail ) /
                 %xF4 %x80-8F 2( UTF8-tail )
   UTF8-tail   = %x80-BF



Related Tickets:

Attachments (8)

29717.php.patch (4.1 KB) - added by askapache 10 years ago.
29717.wp_check_invalid_utf8.patch
29717.patch (4.1 KB) - added by askapache 10 years ago.
29717.patch
29717.2.patch (4.1 KB) - added by askapache 9 years ago.
29717.2.patch
29717.3.patch (4.9 KB) - added by askapache 9 years ago.
29717.3.patch - kitchin
29717.4.patch (5.0 KB) - added by askapache 9 years ago.
29717.4.patch - kitchin2
29717.5.patch (4.3 KB) - added by askapache 9 years ago.
29717.5.patch - mb_convert_encoding preferred over iconv
bug29717.k1.patch (3.3 KB) - added by kitchin 5 years ago.
Add a bytewise regex with a new parameter, remove iconv, speed up empty string check.
bug29717.k2.patch (3.6 KB) - added by kitchin 5 years ago.
Fix use of static override.

Download all attachments as: .zip

Change History (34)

@askapache
10 years ago

29717.wp_check_invalid_utf8.patch

@askapache
10 years ago

29717.patch

#1 @askapache
10 years ago

  • Keywords has-patch dev-feedback added

#2 follow-up: @miqrogroove
9 years ago

Impressive. So the main benefits are 10% faster and more compatibility? Are there any systems currently running WordPress that need this patch? A more concise, big picture description would help.

Also, I learned in feedback from the 4.0 release that we need to specifically test PHP versions less than 5.4.9 and 5.3.19, because they exhibit crashes when PCRE is used to perform certain types of alternation and backtracking. I found that version 5.2.13 is particularly easy to download. It is not necessary to add unit tests for that, but we need to see that if someone posts a 10kb or 100kb block of text that it won't suddenly crash due to a server bug.

#3 in reply to: ↑ 2 @askapache
9 years ago

Replying to miqrogroove:

Impressive. So the main benefits are 10% faster and more compatibility? Are there any systems currently running WordPress that need this patch? A more concise, big picture description would help.

Also, I learned in feedback from the 4.0 release that we need to specifically test PHP versions less than 5.4.9 and 5.3.19, because they exhibit crashes when PCRE is used to perform certain types of alternation and backtracking. I found that version 5.2.13 is particularly easy to download. It is not necessary to add unit tests for that, but we need to see that if someone posts a 10kb or 100kb block of text that it won't suddenly crash due to a server bug.

The updates don't actually change the behaviour of this function unless:

  1. You are one of those with a site with an older pcre lacking utf-8 support, in which case those 4 functions will now correctly filter and check for invalid utf.
  2. You use the strip parameter to actually remove invalid utf for a plugin or theme, in which case it will now work correctly. That was a bug fix.

Some folks have pcre compiled without utf support enabled or with utf-support missing, or disabled, so for them the '/u' doesn't work which results in essentially this entire check being skipped.

This is also somewhat of a security issue, such as the whole IDN domain issues and other utf exploits. The big big picture with this is to update the function to more easily developed and used, it hadn't been updated for quite a while. This should make it easier to update/extend/move this function down the road, I think some people may have wrongly assumed that it was doing more than it was. It's kind of a strange function, to take a string as input and either return it as is, or return a blank string in case of invalid utf-8. But that's actually really clever, it's much safer and faster that way, just not so clear.

I've noticed several plugins like disqus and yoast seo have started to build their own incarnations of this function, this update should help make clear what it is and isn't.

I have tested on PHP 5.2, I approached this with extreme caution to avoid causing any issues. IOW, this function will also work on 5.0. The only reason it wouldn't work for php 4.x is that stripos wasn't available as a builtin zend function until 5.0, but I noticed it's being used in several places in core so.. ( I am still used to having to code backwards for 4.x, so happy that's officially over for WP).

The big change are the 2 new fallbacks to the original preg_match, including the custom regex, which will be the fallback for those with absolutely no utf pcre capability. It has to be a rarity for that to ever actually be needed, but that's the only possible issue I can see with regard to buffer issues or memory problems. preg_match isn't as efficient as a builtin function such as strpos, but it is pretty darn efficient.

The other big change is making the 'strip' parameter work, since it isn't actually being used by any core, it seems to have been forgotten about a little. With it now working, I will start using it in plugins and themes to sanitize utf-8 (because this is super fast). That's actually why I initially started on this.

@askapache
9 years ago

29717.2.patch

#4 follow-up: @kitchin
9 years ago

The checks and ini_set() in the if ($strip ) {} block only need to be run once.

The stripos() is only run once so does not save much and is probably wrong, since it matches 'utf-16', 'utf-7', etc.

Also the inline doc in the patch claims too much, while the old one said too little. Maybe this:

 * @return string If the string is valid UTF-8 or the blog_charset is not UTF-8, the string
 is returned unmodified. Otherwise, an empty string is returned, or optionally the string
 stripped of invalid chars.

@askapache
9 years ago

29717.3.patch - kitchin

#5 in reply to: ↑ 4 @askapache
9 years ago

Replying to kitchin:

The checks and ini_set() in the if ($strip ) {} block only need to be run once.

The stripos() is only run once so does not save much and is probably wrong, since it matches 'utf-16', 'utf-7', etc.

Also the inline doc in the patch claims too much, while the old one said too little. Maybe this:

 * @return string If the string is valid UTF-8 or the blog_charset is not UTF-8, the string
 is returned unmodified. Otherwise, an empty string is returned, or optionally the string
 stripped of invalid chars.

Great feedback kitchin, I have just updated the patch to 29717.3.patch with all of your improvements. Please check it.

#6 follow-ups: @kitchin
9 years ago

Cool stuff. Comments:

(1) I still think the old blog_charset check is clearest. No need to confuse people into having to look up obscure docs. Old code:

in_array( get_option( 'blog_charset' ), array( 'utf8', 'utf-8', 'UTF8', 'UTF-8' ) )

vs. your new code

stripos( $is_utf8, 'utf' ) !== false && strpos( $is_utf8, '8' ) !== false

(2) The WP code base never checks the result of ini_set() or @ini_set() but in this case it seems wise to do so. Hosts can disallow it. Most robust way is probably:

static $mb_convert;
if ( function_exists( 'mb_convert_encoding' ) ) {
  @ini_set( 'mbstring.substitute_character', 'none' );
  $mb_convert = @ini_get( 'mbstring.substitute_character' ) === 'none';
}

I don't imagine anybody is worried about changing that ini value with restoring it, but it should probably be noted in the inline doc as a side effect.

As for WP coding standards nits, WP wants braces on all clauses (if ... {}). Also, no parentheses around function_exists() at line 775.

@askapache
9 years ago

29717.4.patch - kitchin2

#7 in reply to: ↑ 6 @askapache
9 years ago

Replying to kitchin:

Cool stuff. Comments:

(1) I still think the old blog_charset check is clearest. No need to confuse people into having to look up obscure docs. Old code:

in_array( get_option( 'blog_charset' ), array( 'utf8', 'utf-8', 'UTF8', 'UTF-8' ) )

vs. your new code

stripos( $is_utf8, 'utf' ) !== false && strpos( $is_utf8, '8' ) !== false

(2) The WP code base never checks the result of ini_set() or @ini_set() but in this case it seems wise to do so. Hosts can disallow it. Most robust way is probably:

static $mb_convert;
if ( function_exists( 'mb_convert_encoding' ) ) {
  @ini_set( 'mbstring.substitute_character', 'none' );
  $mb_convert = @ini_get( 'mbstring.substitute_character' ) === 'none';
}

I don't imagine anybody is worried about changing that ini value with restoring it, but it should probably be noted in the inline doc as a side effect.

As for WP coding standards nits, WP wants braces on all clauses (if ... {}). Also, no parentheses around function_exists() at line 775.

As the blog_charset check does only run once, I agree that the old code is better. I also added your suggestion to verify that the ini value is correctly set to 'none' as part of the requirement for using mb_convert_encoding if iconv is unavailable.

I also went ahead and added braces, and removed the parentheses from the function_exists statement, nice one.

#8 @askapache
9 years ago

We could also use that same preg_match regex as a preg_replace failsafe for strip in case a machine can't use either iconv or mb_convert_encoding, I don't want to add if that situation rarely happens, since nowhere in core is the strip parameter being used.

Thoughts?

#9 @chriscct7
9 years ago

#11738 was marked as a duplicate.

#10 @askapache
9 years ago

Did some benchmarking on both valid and invalid, super long and normal length strings.

At first I was also using mb_check_encoding, but it would cause max execution time errors even on medium sized strings.

/*
BENCHMARKS ON INVALID STRING (750,000 iterations)
 mb_strlen 16,049,664      750k time  avg from 15k iterations
    preg_match_modifier)   0.73318    0.014659   
   preg_match_backtrack)   0.73956    0.014787 
       htmlspecialchars)   45.36456   0.907278    
     preg_match_pattern)   2.06490    0.041293  
      mb_check_encoding)   CRASHED, IT CHECKS ENTIRE STRING SO TAKES FOREVERRRRRRR

 mb_strlen 2,674,944       750k time  avg from 15k iterations
    preg_match_modifier)   0.76279    0.015250  
   preg_match_backtrack)   0.75758    0.015147
       htmlspecialchars)   0.83401    0.016673   
     preg_match_pattern)   2.15377    0.043068 

 mb_strlen 344             750k time  avg from 15k iterations
    preg_match_modifier)   0.74996    0.014995 
   preg_match_backtrack)   0.73503    0.014697
       htmlspecialchars)   0.70115    0.014019	 
     preg_match_pattern)   2.06986    0.041393


BENCHMARKS ON VALID STRING (750,000 iterations)

    strlen 26,873,856      750k time  avg from 15k iterations
    preg_match_modifier)   0.74948    0.014984
   preg_match_backtrack)   0.75690    0.015133
       htmlspecialchars)   44.17337   0.883453
     preg_match_pattern)   10.71417   0.214273

    strlen 16              750k time  avg from 15k iterations
    preg_match_modifier)   0.79939      0.015984
   preg_match_backtrack)   0.80240      0.016044
       htmlspecialchars)   0.86205      0.017237
     preg_match_pattern)   10.63511     0.212693
*/
class utf_validity {
  public function preg_match_modifier($string) {
    return ( preg_match( '//u', $string ) !== false );
  }
  public function preg_match_backtrack($string) {
    return ( preg_match( '/(*UTF8)/', $string ) !== false );
  }
  public function htmlspecialchars($string) {
    return ( htmlspecialchars( $string, null, 'utf-8' ) != '' );
  }
  public function mb_check_encoding($string) {
    return ( mb_check_encoding( $string, 'UTF-8' ) );
  }
  public function preg_match_pattern($string) {
    static $pattern;
    if ( $pattern == null ) {
      $pattern = '/('
      . '[\xC0-\xC1]' # Invalid UTF-8 Bytes
      . '|[\xF5-\xFF]' # Invalid UTF-8 Bytes
      . '|\xE0[\x80-\x9F]' # Overlong encoding of prior code point
      . '|\xF0[\x80-\x8F]' # Overlong encoding of prior code point
      . '|[\xC2-\xDF](?![\x80-\xBF])' # Invalid UTF-8 Sequence Start
      . '|[\xE0-\xEF](?![\x80-\xBF]{2})' # Invalid UTF-8 Sequence Start
      . '|[\xF0-\xF4](?![\x80-\xBF]{3})' # Invalid UTF-8 Sequence Start
      . '|(?<=[\x0-\x7F\xF5-\xFF])[\x80-\xBF]' # Invalid UTF-8 Sequence Middle
      . '|(?<![\xC2-\xDF]|[\xE0-\xEF]|[\xE0-\xEF][\x80-\xBF]|[\xF0-\xF4]|[\xF0-\xF4][\x80-\xBF]|[\xF0-\xF4][\x80-\xBF]{2})[\x80-\xBF]' # Overlong Sequence
      . '|(?<=[\xE0-\xEF])[\x80-\xBF](?![\x80-\xBF])' # Short 3 byte sequence
      . '|(?<=[\xF0-\xF4])[\x80-\xBF](?![\x80-\xBF]{2})' # Short 4 byte sequence
      . '|(?<=[\xF0-\xF4][\x80-\xBF])[\x80-\xBF](?![\x80-\xBF])' # Short 4 byte sequence (2)
      . ')/';
    }
    return ( preg_match( $pattern, $string ) != 1 );
  }
}

@askapache
9 years ago

29717.5.patch - mb_convert_encoding preferred over iconv

#11 @askapache
9 years ago

Spent a lot of time looking into making this more compatible without sacrificing the speed gains of the new version. Patch 29717.5.patch incorporates the results from the benchmarking, significantly it removes the preg_match custom regex and replaces it with a much quicker and safer htmlspecialchars to check for validity.

The preg_match's are still a tiny bit faster than htmlspecialchars, but in my testing they had the exact same results for testing invalid UTF. And htmlspecialchars is a super core named function from /ext/standard/html.c and has supported this type of checking since before 5.0.

I also determined in the testing of the new strip code (this version prefers mb_convert_encoding over iconv, and uses the mb_substitute_character function instead of ini_set) that the current wp_check_invalid_utf8 function is definitely broken when using the strip parameter. It needs the IGNORE and should also use '@'. As it is now when you run it with the strip parameter turned on and with an invalid utf string, it will return boolean false and trigger some PHP Notices.

PHP Notice:  iconv(): Detected an illegal character in input string in /wp-includes/formatting.php on line 738 
PHP Notice:  iconv(): Detected an incomplete multibyte character in input string in /wp-includes/formatting.php on line 738

Just for fun, here are a bunch of notes from researching this stuff. Please re-test and examine this patch. The notes can be found here: http://pastebin.com/M5jBF8Dz

Last edited 9 years ago by DrewAPicture (previous) (diff)

#12 @askapache
9 years ago

#11175 was marked as a duplicate.

#13 in reply to: ↑ 6 @askapache
9 years ago

Replying to kitchin:

Cool stuff. Comments:

(1) I still think the old blog_charset check is clearest. No need to confuse people into having to look up obscure docs. Old code:

in_array( get_option( 'blog_charset' ), array( 'utf8', 'utf-8', 'UTF8', 'UTF-8' ) )

vs. your new code

stripos( $is_utf8, 'utf' ) !== false && strpos( $is_utf8, '8' ) !== false

(2) The WP code base never checks the result of ini_set() or @ini_set() but in this case it seems wise to do so. Hosts can disallow it. Most robust way is probably:

static $mb_convert;
if ( function_exists( 'mb_convert_encoding' ) ) {
  @ini_set( 'mbstring.substitute_character', 'none' );
  $mb_convert = @ini_get( 'mbstring.substitute_character' ) === 'none';
}

I don't imagine anybody is worried about changing that ini value with restoring it, but it should probably be noted in the inline doc as a side effect.

As for WP coding standards nits, WP wants braces on all clauses (if ... {}). Also, no parentheses around function_exists() at line 775.

Hey kitchin, see any room for improvement on the latest patch? Would love more constructive feedback..

#14 @askapache
9 years ago

yikes i didnt realize trac was lonlier than myspace. Dont take it perspnally but it seems there is a slight lag issue. Or maybe my code is bad or maybe icontributions arent wanted? Or maybe I submitted it wrong? Is this lag ok with everyone and is ot just a part of doing business? Wow. New features are great, but isnt there anyone who desires to put core code improvements ahead of new bells and whistles? lol sorry in advance, well aware this type of complaint wont win me any friends. :)

#15 follow-up: @kitchin
9 years ago

Hi askapache, my Myspace account has lapsed but I did see this and can tell you I only have certain hours to work on Wordpress each week. I don't comment right away since that just clogs up everyone's emails.

#16 in reply to: ↑ 15 @askapache
9 years ago

Replying to kitchin:

Hi askapache, my Myspace account has lapsed but I did see this and can tell you I only have certain hours to work on Wordpress each week. I don't comment right away since that just clogs up everyone's emails.

That'll teach me to drink&corecomment eesh. It is notable that this ticket replaces others, some of which were several years old. Looking forward to your feedback kitchin!

#17 @askapache
9 years ago

FYI, Ive been running WP with this modification in place for 4.0 and 4.0.1, no issues.

#18 @helen
9 years ago

  • Focuses administration removed
  • Version trunk deleted

#19 @askapache
9 years ago

FYI, been running this patch for a couple weeks on 4.1, no issues.

#20 follow-up: @askapache
9 years ago

  • Keywords needs-codex added
  • Severity changed from normal to major

What is the holdup on this? How can I help?

Last edited 9 years ago by askapache (previous) (diff)

#21 @askapache
9 years ago

  • Keywords needs-codex removed
  • Severity changed from major to normal

#22 in reply to: ↑ 20 @helen
9 years ago

Replying to askapache:

What is the holdup on this? How can I help?

I think it's a question of "who has the time and tenacity to tackle this right now?" - I will ask around a bit to see if we can at least get some more feedback, though there's quite a bit going on right at the moment. It's definitely not in my individual wheelhouse.

#23 @desrosj
5 years ago

  • Keywords needs-refresh needs-unit-tests added

Related: #38044.

The current patch is no longer applying cleanly to trunk. @askapache are you able to refresh? I'd also like to see some unit tests here.

@kitchin
5 years ago

Add a bytewise regex with a new parameter, remove iconv, speed up empty string check.

#24 @kitchin
5 years ago

Proof of concept, needs unit tests. Passes my ad-hoc testing with @askapache's test strings.

  1. Speed up testing for an empty string. Indeed stackexchange says 0==strlen($string) is slower than isset($string[0]). But ==$string is almost as fast and matches the WP codebase.
  1. For stripping, iconv() misses some patterns in the test strings, on my platform at least. But the bytewise regex in wpdb::strip_invalid_text() finds them all (4 byte version). So use that.
  1. Add a new parameter $bytewise that controls use of the regex from wpdb. "Bytewise" here means without using "/u".

The new parameter (set to 'always') should solve #38044 by providing a better check than seems_utf8().

By default the patch works the sane as trunk, when $strip is off. For $strip the patch uses the wpdb regex instead of inconv(). Note there's a slight bug in trunk since the return can be null instead of string if inconv() fails, and also inconv() should be @inconv().

Compared to @askapache 29757.5.patch this patch does not try to use '*UTF8' or htmlspecialchars() as fallbacks. The wpdb regex may be slower, but it's only used when "/u" is not available, or for the "not recommended" strip. It's five years later now, so platforms are better, and "not recommended" has been in the codebase longer than that.

Note the code patched has not changed logically since WP 4.0, approx. when this bug started.

(I'm going to post an updated patch that fixes a bug.)

@kitchin
5 years ago

Fix use of static override.

#25 @askapache
5 years ago

Thanks @kitchin !

#26 @askapache
5 years ago

@kitchin I was happy to see some activity after 5 years! Now it's been 5 weeks waiting on what I considered an easy review 5 years ago!!. Something is broken if this is too hard to get reviewed, something is broken if it's not too hard, something is broken. 5 years.

If it's just bad code, that would be a great 10s response. I stopped contributing. I won't contribute in the future. Huge waste of time of effort. 5 years.

Note: See TracTickets for help on using tickets.