PHP v5.4+ Codebase Enhancements

[jaswrks]

Starting with the next release of ZenCache, PHP v5.4+ will be a requirement. I'm opening this issue so that we have a place to list and work on PHP v5.4 improvements; i.e., changes to the codebase that will reduce complexity, improve performance, dependability, and make it easier to maintain.

Referencing: http://php.net/manual/en/migration54.new-features.php
Referencing: http://php.net/manual/en/migration54.functions.php
Referencing: http://php.net/manual/en/migration54.incompatible.php

---

TODO List

• Convert all instances of array() into [] for cleaner formatting and less code in memory.
• Review all calls to ob_start() (the heart of ZC) and update any unnecessary 5.3 fallbacks.
• Convert all Closures into Traits, which will take advantage of autoloading and make ZenCache seemingly more compatible w/ the OPcache extension for PHP. In addition, this will restore the ability for those who are using PhpStorm to more easily navigate the codebase. Converting Closures into Traits should not be that difficult, and should not come with any significant learning curve either, as the methods and location of those files will remain as they are.
• No longer necessary to build $_this references for closure callback handlers.
• No longer necessary to have a fallback for http_response_code().

---

Also Suggested...

Forked to #703.

[jaswrks]

Next Actions (Step 1 of Many) ~ for @renzms

UPDATE: This has been resolved in wpsharks/comet-cache-pro#223

---

• New feature branch in the websharks/zencache-pro repo.
  Please name it feature/635-arrays

• Search the entire codebase (excluding includes/share.php) and look for: array(

• Replace instances of array() with the short-array syntax.

  array(1,2,3)

  becomes...

  [1,2,3]

• Submit PR for review.

---

Example:

$GLOBALS['WebSharks\\HtmlCompressor_early_hooks'][] = array(
    'hook'          => 'css_url()', // Filters CSS `url()`s.
    'function'      => array($this, 'htmlCUrlFilter'),
    'priority'      => PHP_INT_MAX - 10,
    'accepted_args' => 1,
);

becomes...

$GLOBALS['WebSharks\\HtmlCompressor_early_hooks'][] = [
    'hook'          => 'css_url()', // Filters CSS `url()`s.
    'function'      => [$this, 'htmlCUrlFilter'],
    'priority'      => PHP_INT_MAX - 10,
    'accepted_args' => 1,
];

Referencing: http://php.net/manual/en/language.types.array.php

[jaswrks]

Next Actions (Step 2 of Many) ~ for @kristineds

Update: This was completed in wpsharks/comet-cache-pro#222.

---

• New feature branch in the websharks/zencache-pro repo.
  ~ Please DO make this a separate branch.
  Please name it feature/635_this
• Search the entire codebase for instances of: $_this (leading underscore).
• Remove the use of $_this (no longer necessary).
• Submit PR for review.

---

Example:

• You can remove this line entirely.

• Now, looking closer in that file, find places where $_this was being used previously.

  add_action('wp_head', function () use ($_this) {
            $_this->completed_wp_head_action_hook = true;
        }, PHP_INT_MAX); // The very last hook, ideally.

  and change that by removing the unnecessary $_this reference. We can now just use $this (no leading underscore), which is a PHP built-in variable that works without the need for $_this any longer. For instance, I can optimize the above code:

  add_action('wp_head', function () {
            $this->completed_wp_head_action_hook = true;
        }, PHP_INT_MAX); // The very last hook, ideally.

---

We need to do this same thing in a few places throughout the entire codebase. It will be your job to hunt those down, weed them out, and get us up-to-date.

---

Referencing: http://php.net/manual/en/functions.anonymous.php

[jaswrks]

@raamdev Regarding this item listed above...

Adopt the use of all mb_*() functions where applicable.

Can you please give us an OK on Multibyte String support in ZenCache? i.e., is it OK for us to absolutely require every site to be running the mbstring extension for PHP? My vote is yes. http://php.net/manual/en/mbstring.installation.php

[jaswrks]

Next Actions (Step 3 of Many) ~ @raamdev

This was completed in wpsharks/comet-cache-pro#224.

---

• New feature branch in the websharks/zencache-pro repo.
  Please name it: feature/635-http_response_code

• Completely remove this block of code.

• Replace this line of code with the following:

  } elseif (($code = (integer) http_response_code())) {

• Submit PR for review.

[jaswrks]

@kristineds @renzms @raamdev Whoever takes Step 3 should probably the let the others know :-)

[raamdev]

@jaswsinc writes...

Can you please give us an OK on Multibyte String support in ZenCache?

Hmm, that sounds like it's asking for trouble.

PHP.net says:

mbstring is a non-default extension. This means it is not enabled by default. You must explicitly enable the module with the configure option.

If I understand the reason for adding Multibyte String support to ZenCache, it's to improve support for sites that use languages with multibyte character encoding, correct? If so, that does not sound like a high priority to me, certainly not something high enough to warrant the inconvenience of not being able to run ZenCache if your web host happened to not enable the mbstring extension.

My vote is that we either postpone requiring mbstring until a time when there is a greater demand for it, or we make it optional, i.e., if it's available use it; if it's not then fall back on what we have now.

[jaswrks]

My vote is that we either postpone requiring mbstring until a time when there is a greater demand for it, or we make it optional, i.e., if it's available use it; if it's not then fall back on what we have now.

I vote in the other direction, but I understand your concern.

---

If we don't require the mbstring extension, then we simply cannot add full support for UTF-8 string parsing performed by PHP. So this section will need to be removed from the suggestions above and delayed until such time as we can require the mbstring extension.

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 ZenCache, we should:

---

The reason that I vote in the other direction is because:

• The mbstring extension should be enabled by default. It isn't, but nearly all web hosts enable it, because it is used by almost every framework that exists for PHP, including WordPress.
• The reason for us needing the mbstring extension, and that we need to update calls to preg_ functions with the /u flag are outlined here. http://www.phpwact.org/php/i18n/utf-8

You can note that some UTF-8 symbols (e.g., ™, ®, accents, emojis, etc), can cause highly unexpected results whenever we parse strings with functions that are not UTF-8-friendly. This goes by unnoticed for the most part, but there is the chance of random corruption and/or blank pages until we update the codebase in ways that will allow for proper parsing of UTF-8 chars in all scenarios.

[raamdev]

The mbstring extension should be enabled by default. It isn't, but nearly all web hosts enable it,

Right, but if it isn't enabled then ZenCache becomes unusable. If we're going to require it to run ZenCache, then we need to do the same thing we did with the PHP 5.4 requirement: come up with a plan for announcing the change to anyone who is currently running ZenCache on a site that doesn't have mbstring installed, and let them know that an upcoming version will require it.

Collecting that information via the stats logger to see how many people don't have mbstring installed would also be a good step if we are to force that requirement.

The other option would be to leave a fallback in place, so that we use it if it's available. Maybe we could create a wrapper function that uses the appropriate call depending on the availability of mbstring?

[raamdev]

@jaswsinc Ping. ↑

[jaswrks]

My opinion is that we need to use mbstring in ZenCache to avoid unforeseen issues and improve reliability of the code. Long ago, I got into a bad habit of leaving this out of a lot of PHP code that I've written in order to avoid that dependency, but now that WordPress allows for emojis, the likelihood of a conflict occurring is increased by a lot. Without mbstring, the plugin will be unstable on sites that use UTF-8 chars; e.g., blank pages, random corruption, broken HTML tags, etc.

See: http://www.phpwact.org/php/i18n/utf-8

As it exists at the moment, this is a design flaw in ZenCache. So, if you don't have mbstring, you need to install it so that ZenCache can perform as expected.

Collecting that information via the stats logger to see how many people don't have mbstring installed would also be a good step if we are to force that requirement.

I don't have anything against this if you'd like to do that. I'll let you make the call on this.

The other option would be to leave a fallback in place, so that we use it if it's available. Maybe we could create a wrapper function that uses the appropriate call depending on the availability of mbstring?

That's what WordPress does in a few places also. However, the behavior of the plugin will not be what is expected if you don't have mbstring. So I think failing gracefully is a bad thing. If you don't have mbstring installed, you need to install it to avoid unexpected parsing problems.

If it were 2009 I'd be for this approach; i.e., failing gracefully; but, I vote against taking that route today, because nearly all hosting companies include mbstring. I don't think I've seen a PHP installation in the last few years without mbstring.

---

Another option would be to add this as a required extension. The WP PHP RV checks already support checking for required PHP extensions, so I think we could just add mbstring as a formal requirement.

[jaswrks]

Example implementation of websharks/wp-php-rv where we require specific PHP extensions also.
https://github.com/websharks/wp-sharks-core/blob/000000-dev/plugin.php

[raamdev]

That's what WordPress does in a few places also. However, the behavior of the plugin will not be what is expected if you don't have mbstring. So I think failing gracefully is a bad thing. If you don't have mbstring installed, you need to install it to avoid unexpected parsing problems.

If it were 2009 I'd be for this approach; i.e., failing gracefully; but, I vote against taking that route today, because nearly all hosting companies include mbstring. I don't think I've seen a PHP installation in the last few years without mbstring.

I agree. Since the plugin simply won't behave as expected in some scenarios where it should behave properly, making mbstring a required extension to run ZenCache makes sense.

---

My only concern is not knowing how much of our userbase is currently happily using ZenCache without the mbstring extension (and not experiencing any problems because their site doesn't use any special UTF-8 chars).

If there is a significant percentage of our userbase that fits that description and we do a release that suddenly prevents ZenCache from working unless they have that extension installed, that's not very user-friendly and could lead to a lot of frustration.

On the other hand, if we just enforce this requirement over the course of 2 releases, we can do a release that shows a warning notice to site owners running ZenCache on servers without the mbstring extension (along with a link to a KB Article), and then on the subsequent release we can require mbstring.

That would be acceptable to me. I just don't like knowingly introducing something that might prevent a large percentage of our userbase from using the plugin without some sort of warning ahead of time.

---

So, my vote is for delaying this mbstring work until we've properly notified users who don't have the extension installed that the following release will require it. I'm planning to do a ZenCache GA release tomorrow and we could probably squeeze that first notice into that release.

[jaswrks]

Sounds good. I think a notice and warning is fair enough. I don't see this impacting very many people at all; so running through stats to check the percentages may not be worth the time. However, running stats wouldn't hurt anything either. Whatever you feel best about is fine with me.

As long as we can start using mb_ functions at some point in the near future.

---

If we add this to the WP PHP RV check, then it's not like it will bring down a site anyway. It just won't allow the new version of the plugin to be used until they satisfy that dependency.

[raamdev]

@jaswsinc I'm getting ready to add that notice for today's GA release. Doing a check if extension_loaded('mbstring') should be sufficient, correct?

[raamdev]

Checking off this line item above:

• Convert all Closures into Traits

PR wpsharks/comet-cache-pro#222 (Convert to PHP Traits) has been merged into 000000-dev.

[raamdev]

Checking off this line item above:

• No longer necessary to build $_this references for closure callback handlers.

PR wpsharks/comet-cache-pro#222 fixes that.

[raamdev]

Noting that we can now continue with the conversion to utilize mb_*() functions where applicable. The notice to users indicated that after March 1st we will require the mbstring extension. Also adding the following line item above:

• Ensure that WP PHP RV requires the mbstring extension to prevent issues with sites that don't have it installed.

[raamdev]

Review all calls to ob_start() (the heart of ZC) and update any unnecessary 5.3 fallbacks.

There are three occurrences of ob_start():

• https://github.com/websharks/comet-cache-pro/blob/160227/src/includes/closures/Plugin/WcpEvalUtils.php#L27
• https://github.com/websharks/comet-cache-pro/blob/160227/src/includes/closures/Ac/PostloadUtils.php#L197
• https://github.com/websharks/comet-cache-pro/blob/160227/src/includes/closures/Ac/ObUtils.php#L220

@jaswsinc Do these need to be updated?

[jaswrks]

Do these need to be updated?

Nope. Those all look good. False-alarm on that one. Looks like no changes are necessary with respect to ob_start(). I think I listed that because I was unsure. There were some changes to the parameters to ob_start() in PHP 5.4, but since we dropped the locked output buffer idea, this doesn't seem to impact Comet Cache at all now. Looks good as-is.

[raamdev]

@jaswsinc A new issue has been opened to add support for Multibyte Strings: #703

Unless there's anything else you feel needs to be done as part of this GitHub issue, I'm going to close this as done.

[jaswrks]

Great! :-)

[raamdev]

Next Release Changelog:

• Enhancement: Several PHP 5.4+ enhancements, most notably a conversion from PHP Closures to PHP Traits. See Issue #635.

[raamdev]

Comet Cache v160416 has been released and includes changes from this GitHub Issue. See the v160416 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 (#635).