Make WordPress Core

Opened 7 years ago

Closed 11 days ago

#42264 closed enhancement (maybelater)

Systematic way of dealing with compat code and polyfills

Reported by: schlessera's profile schlessera Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: close
Focuses: performance Cc:

Description (last modified by westonruter)

The way compatibility code and fallback/polyfill functionality is currently handled has a few issues:

  • As everything resides in one big file, all of the code is parsed every time.
  • As everything resides in one file, problems like the PHP 7.2 parsing error for the autoload polyfill can crop up (as the polyfill is written with now deprecated code).
  • If the requirements change, it is non-trivial to remove unneeded code again.

I'd like the suggest a more systematic way of loading the compatibility layer. The basic premise is that the PHP version of the current server is detected, and then an individual compatibility file is loaded for each version that is not already supported by the server.

This provides a clean way of structuring the compatibility layer, giving a good overview of what is needed when, and what can be discarded. It also only loads the code that is needed.

Here's the main mechanism that would make this work:

<?php
// Check the PHP version and load the corresponding compatibility files.
// The fall-throughs (missing breaks) are intentional, as this makes sure that
// all compat files - starting from the first required one - will be loaded.
switch ( substr( PHP_VERSION, 0, 3 ) ) {
        case '5.2':
                require ABSPATH . WPINC . '/compat/php-5.2.php';
        case '5.3':
                require ABSPATH . WPINC . '/compat/php-5.3.php';
        case '5.4':
                require ABSPATH . WPINC . '/compat/php-5.4.php';
        case '5.5':
                require ABSPATH . WPINC . '/compat/php-5.5.php';
        case '5.6':
                require ABSPATH . WPINC . '/compat/php-5.6.php';
        case '7.0':
                require ABSPATH . WPINC . '/compat/php-7.0.php';
        case '7.1':
                require ABSPATH . WPINC . '/compat/php-7.1.php';
        case '7.2':
                require ABSPATH . WPINC . '/compat/php-7.2.php';
        default:
                require ABSPATH . WPINC . '/compat/default.php';
}

Note the fall-throughs of the case statements. As an example, if the current server would be running PHP 5.6, the above mechanism would load the compatibility files for 5.6, 7.0, 7.1 and 7.2.

Inside of the individual files, you'd have fallbacks and polyfills, that are needed for the version it resides in and all previous versions.

Attachments (1)

42264.1.diff (33.9 KB) - added by schlessera 7 years ago.
Sample implementation of the discussed concept

Download all attachments as: .zip

Change History (10)

@schlessera
7 years ago

Sample implementation of the discussed concept

#1 @westonruter
7 years ago

  • Description modified (diff)

#2 @ayeshrajans
7 years ago

Hi everyone - is there any indication if this approach will be used in core? If that's the case, I'm willing to dedicate my time to wrote the polyfills (where possible to do in userland code of course) and maintain it.

#3 @jrf
7 years ago

@schlessera I've been thinking about this a little more and while I support the basic premise of this proposal - splitting out the back-compat functionality into several files which are selectively loaded -, I don't think the currently proposed logic would work.

A significant number of the back-fills are related to PHP extensions - in contrast to the PHP Core -.
Depending on the PHP version, certain extensions ship with PHP by default. However, this is no guarantee that those extensions will actually be available in that PHP version or not available in versions where the extension was not (yet) included by default.

  • Webhosts, or any PHP installer for that matter, do not have to use the default distribution.
  • Some webhosts backport security fixes to old PHP versions and at times, these fixes may contain functionality which has been backfilled.
  • A custom PHP compilation using PECL extensions may be used.
  • etc etc

So the fact that WP is running on a system running PHP 7.2, is no guarantee that the Sodium extension is available, PHP 5.x is no guarantee that bcmath is available (shipped with PHP since 4.0.4), and running PHP 7.0 is no guarantee that Sodium will *not* be available.

Over the last x years I've been amazed (and frustrated) more times than I can count by bug reports for plugins where it turned out that a non-default PHP install missing some essential extension was the underlying cause of the issue.

So, having said that, I think a more extension based approach is needed here using extension_loaded() to check whether, for instance, the back-fills related to MBString need to be loaded or not.

Presuming all back-fill related files will live in a wp-includes/phpcompat directory, I imagine the loading mechanism could be something along the lines of (pseudo-code, untested, just to demonstrate the idea):

<?php
$php_backfills = array(
        'CSPRNG',
        'MbString',
        'Sodium',
        'Curl',
);

require_once 'wp-includes/phpcompat/core-functions-and-constants.php';

foreach ( $php_backfills as $extension ) {
        if ( extension_loaded( $extension ) === false ) {
                require_once 'wp-includes/phpcompat/' . $extension . '.php';
        }
}

Where necessary, this could be enhanced with a call to phpversion( $extension ) to check that the extension complies with a minimum required version of that extension.

Version 0, edited 7 years ago by jrf (next)

#4 @ayeshrajans
7 years ago

👍👌 for extension_loaded calls to detect availability. That really makes the code more readable too.
<bike-shedding>
There are few arguments in outside PHP communities to use function_exists vs extension_loaded - function_exists is tiny bit faster and functions can be disabled in php.ini where extension_loaded would happily return true, but function_exists doesn't.

</bike-shedding>

#5 @jrf
7 years ago

@ayeshrajans All individual back-fills should still be wrapped in function_exists(), interface_exists(), defined() calls, just like they are now.

The extensions_loaded() logic example was just intended to regulate which back-fill files to load.

and functions can be disabled in php.ini where extension_loaded would happily return true, but function_exists doesn't.

You have a point there which needs further thought, as that would be a argument to have the compat bootstrap file have all the function_exists() checks and have the back-fill files just add individual functions, though that would also necessitate each back-fill to be in their own file, which might be going over the top a bit with modular loading.

#6 @schlessera
6 years ago

@jrf The intent of the ticket was not necessarily to provide a more granular way to load the backcompat code, as it was to make maintenance easier with regards to how we hopefully manage to more closely match the PHP update velocity in the long run.

For extensions that are optional, you will always have to at least load the code that will check whether the extension is active and can be used. The check might even sometimes be complicated, as even extensions are not always compiled in the same way.

For extensions that have become required at one point, you always load the polyfill except for when you've reached the minimum PHP version where it became standard.

The setup I suggested basically makes it very easy to get rid of redundant code every time we bump the PHP minor version. So, when we bump PHP minimum version from PHP 5.2 to PHP 5.3 hopefully soon, we just delete the /compat/php-5.2.php file and that's it. PHP is moving faster and faster, and collecting a huge amount of compat code without a mechanism to get rid of it as needed, and without doubts about what to remove seems useful.

I see no reason why we couldn't combine both approaches, to have PHP-version-based files and extension-based files.

#7 @jrf
6 years ago

I see no reason why we couldn't combine both approaches, to have PHP-version-based files and extension-based files.

@schlessera Agreed 👍.

For extensions that have become required at one point, you always load the polyfill except for when you've reached the minimum PHP version where it became standard.

My point was more that this statement can not be relied on as PHP can be (and I've seen it happen) compiled without the required extensions.

#8 @jorbin
8 weeks ago

  • Focuses performance added
  • Keywords close added

During the most recent PHP version upgrade, only one function and two constants were able to be removed.

Taking a look at the current state of the file:

  1. 3 constants tied to specific PHP Versions
  2. 7 Functions tied to specific PHP Versions
  3. 5 Functions tied to PHP Extentions
  4. 5 internal helper functions
  5. 1 check if sodium_compat should be loaded.

At this point, I think that updating the file . Unless there are specific performance improvements that can be identified by reorganizing to do separate includes, I think the maintenance burden hasn't been too much thus far and doesn't appear to be a need on the horizon that will expand this burdon.

Adding the close keyword since I think this might be best as maybelater depending on how this function grows. Adding the performance keyword in case someone wants to run some performance tests but I suspect it's a micro optimization at best.

#9 @johnbillion
11 days ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.