@cmb69 i haven't test it yet, but weren't it safer to use something like php_next_utf8_char() if you wanted to get a valid UTF-8 char? Like strlen(php_next_utf8_char(...)) or advance by 1 byte, if not.

The 'u' modifier expects an UTF-8 string per se. If the charset is broken, so the behavior is undefined. IMHO there should be more error check, so the behavior were more predictable at the end.

Thanks.

@weltling I wasn't aware of the existance of php_next_utf8_char(). I'll have a look at it, and add some tests for broken UTF-8 strings.

@cmb69, great, let's see to which conclusion you came, i'll be testing subsequently then.

Thanks.

@weltling At this point pcre_exec will have already checked that the string is valid UTF-8.

Patch looks good to me. I'd only suggest extracting the duplicate code into an inline function.

@weltling @nikic I've did some very quick experiments with php_next_utf8_char() a while ago, but that didn't work out well (the API is overly complex for this purpose). And if UTF-8 validity is guaranteed at this point, the simple while loop is sufficient, and faster anyway.

Wrt. inlining: is zend_always_inline the proper way to declare this?

@cmb69, but did you at least ensured that if you pass an invalid utf8, it works as you expect? @nikic, where the check happens then?

@nikic, to me it looks like it operates on the plain subject passed.

Thanks.

@weltling For php_pcre_match_impl() the UTF-8 validity is supposed to be guaranteed further above. For php_pcre_replace_impl() it's here.

@cmb69, ack )

@weltling @nikic Please review.

@cmb69 tested and don't see any issue. I'd still suggest to add a test with invalid utf8 though. Fe, i've modified the string 'áéíóú' in one of the tests just appending 'ü' encoded with cp1252, and that already delivers NULL. Maybe that's nitpicking though :) But otherwise tests are good.

Thanks.

@weltling I've added some tests for invalid UTF-8 sequences. You're right: better safe than sorry. :)

I suppose the patch should be merged into PHP 5.6 and master.

@cmb69, if there are no other comments, i guess it can be merged.

Thanks for the effort :)

Comment on behalf of cmb at php.net:

Merged to 5.5, 5.6 and master.