Thanks for the feedback @danielbachhuber . I've updated based on your comments.

@scribu I see your point. Plain substr()as written would not be encoding safe. mb_substr() and mb_strlen() would definitely be the preferred method, but if that extension is not enabled on a PHP installation then it results in a fatal error.

Would it be safe to assume UTF-8 encoding in cases where mb_substr() is not installed? If that were the case then:

<?php
$str = 'sécurité';
echo utf8_encode( substr( utf8_decode( $str ), 0, 2 ) ) . "\n";`

Outputs:

sé

and could be used as a fallback for mb_substr() to avoid the fatal error. Would that be acceptable?

Would it be safe to assume UTF-8 encoding in cases where mb_substr is not installed?

No, I don't think that's a safe assumption.

However, I think mb_substr() usage in the search-replace command is an optimization. If mb_substr() isn't available, you could make the command fall back to the slow method.

Edit: Oh, it seems mb_strlen() is used in php-cli-tools: #53. Welp.

What I'd like to have for php-cli-tools is:

1. Use mb_strlen() if it exists.
2. If not 1, use strlen(). But, try and detect if there is encoding in the string, and trigger a PHP notice if there is.

For WP-CLI, we can fall back to strlen()

For WP-CLI, we can fall back to strlen()

I spoke too soon. We don't use mb_strlen() or mb_substr() in WP-CLI. Your original fatal error is because of php-cli-tools

If not 1, use strlen(). But, try and detect if there is encoding in the string, and trigger a PHP notice if there is.

Since these functions are just used for displaying data, as opposed to manipulating data that will be sent to a database, I think that's acceptable. The function descriptions are still incorrect, though.

The function descriptions are still incorrect, though.

I'll try to add that clarification in an update of the pull request that also includes @danielbachhuber recommendation for the mb_strlen() fallback

Thanks @jesseoverright. I'd love to get this sorted out in the next couple of days so we can ship 0.17.0

Sounds good. I'll submit an updated pull request tomorrow.

On Sep 6, 2014, at 1:36 PM, Daniel Bachhuber [email protected] wrote:

Thanks @jesseoverright. I'd love to get this sorted out in the next couple of days so we can ship 0.17.0

—
Reply to this email directly or view it on GitHub.

Function descriptions are updated. Also added iconv() (available by default in PHP) to trigger a PHP notice if non-ascii encoding is present (in cases where mb_substr() & mb_strlen() are missing). mb_detect_encoding() would of course be preferred, but wouldn't be available in this edge case.

Also included tests for \cli\safe_subst()

👍