Support for Multibyte Strings

[raamdev]

Forked from #635.

@jaswsinc writes...

We should do a better job (even better than WP itself) in dealing with UTF-8. Given the amount of string manipulation that occurs in Comet Cache, we should:

• Adopt the use of all mb_*() functions where applicable.
• Always pass the /u flag to any preg_*() functions that we use.
• Ensure that WP PHP RV requires the mbstring extension to prevent issues with sites that don't have it installed.

This work can probably be moved to a separate issue though; i.e., we should have a separate issue that will be for UTF-8 enhancements. See this article.

Related GitHub issue: #640 (comment)

---

Also noting here at an announcement was made in Comet Cache v160222 to any site owner that did not have the mbstring extension installed that the PHP mbstring extension would be required by Comet Cache after March 1st.

[jaswrks]

• [x]

A first and very easy step here would be to search for all calls to:

• preg_match
• preg_replace
• preg_replace_callback

and on the end of the regex pattern add a u modifier.

e.g., /pattern/ becomes /pattern/u
e.g., /pattern/i becomes /pattern/ui

[jaswrks]

• [x]

Next, we need to search for instances of:

• strpos
• stripos

and a decision needs to be made that is based on the following.

• If strpos is being used to detect the number of bytes, leave it as-is.
• If strpos is being used to detect the number of characters (or to search for another string, most prevalent in Comet Cache), change it to mb_strpos or mb_stripos respectively.

[jaswrks]

• [x]

Next, search for:

• strtolower
• strtoupper

And change all of those to mb_strtolower() or mb_strtoupper() respectively.

[jaswrks]

• [x]

Same thing for substr; i.e., should become mb_substr.

[jaswrks]

• [x]

Next, we will need to see which other UTF-8-incompatible functions we are using in Comet Cache; i.e., see which functions we use in PHP core for which there is no UTF-8 mb_* variation available. In those cases, we will need to build a replacement of our own as a polyfill.

e.g., There is no mb_str_ireplace, and there is no mb_str_pad() either. PHP is also missing mb_substr_replace(). I'm not sure if Comet Cache needs any of these (or others), but we need to dig through and see which might need polyfills.

Examples of a UTF-8 compatible polyfills:

• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/SubstrReplace.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/StrCaseCmp.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/StrPad.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/StrRev.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/StrSplit.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/Trim.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/UcFirst.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/UcWords.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/MbLcFirst.php
• https://github.com/websharks/core/blob/000000-dev/src/includes/classes/Core/Utils/ReplaceOnce.php

[jaswrks]

Referencing full list of existing mb_ functions provided by the mbstring extension.
http://php.net/manual/en/ref.mbstring.php

[jaswrks]

• Noting that the trim(), rtrim() and ltrim() PHP functions are generally safe to use on multibyte data. However, if we find instances of these functions where we are attempting to trim characters outside of the ASCII range, we will need to use preg_replace() with the /u modifier instead.

[raamdev]

If strpos is being used to detect the number of bytes, leave it as-is.
If strpos is being used to detect the number of characters (or to search for another string, most prevalent in Comet Cache), change it to mb_strpos or mb_stripos respectively.

Can you give an example of those two? I'm not seeing anything that looks like strpos() is being used to detect the number of bytes...

[jaswrks]

• [x]

Sorry, I stated that incorrectly. What I should have been talking about there was strlen() instead of strpos(). All instances of strpos() and stripos() should be updated to their mb_ counterparts.

---

Examples of strlen() looking for (or testing) the number of bytes:

if(!strlen($variable)) {
   // Empty.
}

header('Content-Length: '.strlen($export));

Examples of strlen() looking for the total number of characters.

if (strlen($string) > $max_length) {

$hook = GLOBAL_NS.substr($hook, strlen('zencache'));

[raamdev]

@jaswsinc writes...

Noting that the trim(), rtrim() and ltrim() PHP functions are generally safe to use on multibyte data. However, if we find instances of these functions where we are attempting to trim characters outside of the ASCII range, we will need to use preg_replace() with the /u modifier instead.

Can you give me an example of where we're "attempting to trim characters outside of the ASCII range"? Trim functions are usually used on variables that have hard-to-predict (dynamic) values, so this seems like a case of challenging best-guess work.

[jaswrks]

Example where it should be updated:

$string = trim($string, '®');

Should become:

$string = preg_replace('/^®+|®+$/u', '', $string);

I'm not sure if there are any cases like this in Comet Cache, so there might be nothing to do.

---

Examples that are fine like they are:

$string = trim($string);
$string = rtrim($string, '/');
$string = ltrim($string, '/');
$string = trim($string, '-');

[raamdev]

Reviewed all instances of trim(), rtrim() and ltrim(); not seeing anything that needs to be changed.

[raamdev]

@jaswsinc writes in #703 (comment)...

There is no mb_str_ireplace, and there is no mb_str_pad() either. PHP is also missing mb_substr_replace(). I'm not sure if Comet Cache needs any of these (or others), but we need to dig through and see which might need polyfills.

I reviewed occurrences of str_ireplace() and substr_replace() in the codebase and none seemed to require Multibyte replacements.

I think the work to add support for multibyte strings is complete except for the analysis of any other areas where we might need polyfills. Would you mind taking a look when you get a chance and let me know if I missed anything here?

Once you're done, I think wpsharks/comet-cache-pro#225 is ready to merge if you agree (all that's left is to add the check to WP PHP RV).

[jaswrks]

Outstanding! I just reviewed the changes that you introduced and those all look great to me. In particular, I think that this line is going to be a very important fix that will prevent corruption of the wp-config.php file in certain cases where it contains UTF-8 chars. I've seen that reported now and again in both CC and in s2Member, and I think this will fix that.

I'm taking a closer look now and the rest of the codebase, so I will post another update shortly. This file in particular needs some work; i.e., substr_replace() there is problematic. As you mentioned, a polyfill will be needed, or an alternate approach maybe.

[raamdev]

@jaswsinc writes...

I'm taking a closer look now and the rest of the codebase, so I will post another update shortly

Ping. ↑ 😄

[jaswrks]

I reviewed occurrences of str_ireplace() and substr_replace() in the codebase and none seemed to require Multibyte replacements.

Anything which currently uses str_ireplace() can be updated to use preg_replace('//ui', ...) instead, for unicode compatibility. So for instance: https://github.com/websharks/comet-cache-pro/search?utf8=%E2%9C%93&q=str_ireplace

$wp_path_rel_to_home = str_ireplace($home, '', $siteurl); /* $siteurl - $home */

Becomes:

$wp_path_rel_to_home = preg_replace('/'.preg_quote($home, '/').'/ui', '', $siteurl); /* $siteurl - $home */

That way if the URL contains a multibyte character it won't get mangled when doing string replacements.

In WordPress (as it stands), this is not going to do anything super useful at the moment, because WordPress is not perfect in the way it deals with UTF-8 yet, which means that $home containing unicode may break, regardless, in various areas of the WP core.

However, from my perspective it's the difference between wrong and right in the way this is handled. As WordPress improves its support for unicode throughout, it will make it possible for WordPress home/blog URLs to contain unicode characters (e.g., emojis) and when that happens we want Comet Cache to be prepared for it.

---

I have some other comments I'll post shortly.

[jaswrks]

This line can become:

$cache_dir_tmp_regex = preg_replace('/'.preg_quote(preg_quote($cache_dir.'/', '/'), '/').'/ui', '', $cache_dir_tmp_regex, 1);

This line can become:

$_host_cache_dir_tmp_regex = preg_replace('/'.preg_quote(preg_quote($_host_cache_path.'/', '/'), '/').'/ui', '', $_host_cache_dir_tmp_regex, 1);

This line can become:

$_host_cache_dir_tmp_regex = preg_replace('/'.preg_quote(preg_quote($_host_cache_dir.'/', '/'), '/').'/ui', '', $_host_cache_dir_tmp_regex, 1);

---

And then, this file could be removed and/or deprecated, because those methods are no longer used by anything.

[jaswrks]

The stripos() examples in this file could be updated to show mb_stripos().
https://github.com/websharks/comet-cache-pro/blob/feature/703/src/includes/templates/ac-plugin.txt

[jaswrks]

These lines can be replaced with:

$advanced_cache_contents = preg_replace(
    [
        '/'.preg_quote("'%%".GLOBAL_NS.'_'.$_option."%%'", '/').'/ui',
        '/'.preq_quote("'%%".GLOBAL_NS.'_'.preg_replace('/^cache_/ui', '', $_option)."%%'", '/').'/ui',
    ],
    $_value,
    $advanced_cache_contents
);

[jaswrks]

This line can be replaced with:

$advanced_cache_contents = preg_replace('/'.preg_quote("'%%".GLOBAL_NS."_PLUGIN_FILE%%'", '/').'/ui', $plugin_file, $advanced_cache_contents);

[jaswrks]

In this file there are a few instances of strtolower() that still should be changed to mb_strtolower().

[jaswrks]

In this file also. Convert all instances of strtolower() into mb_strtolower().

[jaswrks]

Everything else looks great. I'd like to do another round of testing once the above changes are implemented though.

[raamdev]

@jaswsinc This is ready for another round of tests. Your latest suggestions have been committed in wpsharks/comet-cache-pro@edadf24.

[jaswrks]

@raamdev The code sample I gave here has a misspelled word preq instead of preg.

[raamdev]

a misspelled word preq instead of preg.

Ha, glad somebody caught that. 😳

[raamdev]

@jaswsinc writes...

Everything else looks great. I'd like to do another round of testing once the above changes are implemented though.

Were you able to run those tests? Can I merge now, or would you like to do some more testing?

[jaswrks]

I'm running some additional tests now...

[jaswrks]

I ran some tests on this branch and I haven't seen any problems thus far. Looks good-to-go.

[raamdev]

Next Release Changelog:

• Enhancement: Added full support for UTF-8 (multibyte strings). This release adds full support for UTF-8 throughout the Comet Cache codebase, greatly enhancing Comet Cache's ability to deal with file paths and URLs that may contain UTF-8 characters. Props @jaswsinc. Issue #703.

[raamdev]

Comet Cache v160706 has been released and includes changes from this GitHub Issue. See the v160706 announcement for further details.

---

This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#703).