BC issue with mixed versions between WP Request and WP-CLI Request

[schlessera]

Summary

When WordPress uses strictly-typed arguments in constructors dealing with Requests, they break with Requests version mismatches, as the new objects do not inherit the old objects.

Given the following code sample

As an example, let's consider installing WP Core with WP-CLI. This is only a single example for a bigger problem.

wp core install ...

I'd expect the following behaviour

I'd expect this to work successfully, and WP-CLI doing what is needed to put WP Core into an installed state and then outputting:

Success: WordPress installed successfully.

Instead this happened

With Requests v2 bundled with WP-CLI, this fatals:

thrown in /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-includes/class-wp-http-requests-response.php on line 42
Fatal error: Uncaught TypeError: WP_HTTP_Requests_Response::__construct(): Argument #1 ($response) must be of type Requests_Response, WpOrg\Requests\Response given, called in /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-includes/class-http.php on line 397 and defined in /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-includes/class-wp-http-requests-response.php:42
Stack trace:
#0 /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-includes/class-http.php(397): WP_HTTP_Requests_Response->__construct()
#1 /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-includes/class-http.php(626): WP_Http->request()
#2 /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-includes/http.php(162): WP_Http->get()
#3 /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-admin/includes/upgrade.php(522): wp_remote_get()
#4 /tmp/wp-cli-test-run--61afd4a9c43888.66717688/wp-admin/includes/upgrade.php(113): wp_install_maybe_enable_pretty_permalinks()
#5 vendor/wp-cli/core-command/src/Core_Command.php(618): wp_install()
#6 vendor/wp-cli/core-command/src/Core_Command.php(401): Core_Command->do_install()
#7 [internal function]: Core_Command->install()
#8 php/WP_CLI/Dispatcher/CommandFactory.php(100): call_user_func()
#9 [internal function]: WP_CLI\Dispatcher\CommandFactory::WP_CLI\Dispatcher\{closure}()
#10 php/WP_CLI/Dispatcher/Subcommand.php(491): call_user_func()
#11 php/WP_CLI/Runner.php(406): WP_CLI\Dispatcher\Subcommand->invoke()
#12 php/WP_CLI/Runner.php(429): WP_CLI\Runner->run_command()
#13 php/WP_CLI/Runner.php(1210): WP_CLI\Runner->run_command_and_exit()
#14 php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start()
#15 php/bootstrap.php(78): WP_CLI\Bootstrap\LaunchRunner->process()
#16 php/wp-cli.php(27): WP_CLI\bootstrap()
#17 php/boot-fs.php(17): require_once('...')
#18 {main}

Additional context

WP-CLI bundles its own version of Requests, as it also needs to make remote requests when the WP site is missing or broken.

In the example above, what happens is that the BC layer is being triggered by WordPress Core as expected and allows the old classes and methods to be used in older WP core versions.

However, WP Core uses strict typing based on class in some constructors and methods. As the BC layer is built with class aliases, and not with extensions, the type hint is wrong and fatals. Although, the WpOrg\Requests\Response is aliased to Requests\Response, using a trsict type-hint does not seem to trigger the BC layer and so the class alias was not loaded even though it would be needed.

Tested against develop branch?

• I have verified the issue still exists in the develop branch of Requests.

[schlessera]

I will now look into adding a check in WP-CLI that manually triggers the BC layer for all needed classes when it can detected this is needed by WP. I think this should solve the issue for WP-CLI.

I wanted to create the above issue in here nevertheless, as other consumers might be affected, and there might be a fix we can apply within Requests itself.

The obvious fix would be to remove the final again on the classes and have the BC layer extend the new versions, as we had initially discussed, as I think that would just work out-of-the-box across all mismatched version combinations. TBD.

[jrfnl]

@schlessera Couple of questions/observations:

• You mention WPCLI shipping with its own copy of Requests. Does it use PHP_Scoper for that copy ? If not, why not ?
  When I read the above correctly, this sounds like two different versions of Requests being used at the same time and intermingling without scoping to differentiate them (WPCLI using different version from WP Core, WPCLI should use the version included with WPCLI, WP Core should use the version included with WP Core), which is bound to be a recipe for disaster no matter what.
• The BC-layer is one way only: the old class names are aliased to the new names.
  Using the new names with an old install won't work.
• The BC-layer with the extends which we initially had, was only for the Composer autoloader. WordPress uses (and always has used) the custom Autoloader, not the Composer autoloader.

[schlessera]

@jrfnl Yes, currently WP-CLI is using Requests without namespace scoping. I've tried integrating PHP-Scoper, but it breaks more than it fixes in this case, unfortunately. WP-CLI still is forced to adhere to the extension mechanism of WordPress, and that also means that WP-CLI needs to be able to execute WordPress plugins and themes.

The only solution I can think of right now is to load the WP Requests for all commands that need to actually load WP itself, and use its own bundled version if WP is not to be loaded.

Given that Core has postponed the adoption of Requests v2, I'll push the WP-CLI v2.5.1 release for PHP 8.1 compat without the Requests v2 upgrade. This buys me time to look further into how this can be properly resolved, and hopefully we'll be able to properly test already early in the cycle.

[jrfnl]

If nothing else, you can possibly use the version constant to figure out in strategic places which version of Requests has been loaded ?

[jrfnl]

Conclusions from analyzing this issue today in a video call with @schlessera and @alpipego:

The problem occurs in the following situation:

• If during a "page load", some of the "old" Requests classes get loaded into memory
• AND subsequently during that same "page load", the new Requests Autoloader gets loaded
• AND subsequently an instance of a class from the "new" Requests get passed to a function call with a type declaration referencing an "old" Requests class
• AND that old class was one of the classes already in memory
• that's when the error occurs

The reason is as follow:

• As the "old" Requests class is already in memory, the new Autoloader doesn't get triggered for the type declaration and therefore the class alias does not get created.
• Without the class alias, the instance of the "new" Requests class is not recognized as being the same as the "old" class.

The underlying problem is ultimately that during the same "page load" a mix of two different versions of Requests is being used.

The suggested solution for projects which expect to encounter different (major) versions of Requests during the same "page load" and don't isolate the separate projects, is to preload the class aliases for the old Requests classes in a bootstrap file.

If the names of the "old" Requests classes aliased to the "new" classes are in memory, the "old" Autoloader won't get triggered, the class aliases can do their job and effectively only one version of Requests (the new version) will be used during the "page load".

[jrfnl]

Just thinking - to make it easier to implement the above mentioned bootstrapping in projects which have to deal with mixed Requests versions, we could add a public static Autoload::getDeprecatedClasses() method which just returns the value of the private Autoload::$deprecated_classes property.

In a project bootstrap, a project could then do something along the lines of the below (untested, but should work):

require_once 'path/to/Requests/src/Autoload.php';

// Make sure the autoloader is registered first as otherwise you may run into trouble on PHP 8.1.
// See: https://news-web.php.net/php.internals/115549
WpOrg\Requests\Autoload::register();

// Silence deprecations.
// Depending on when the bootstrapping is done, you may need to wrap this in a `if (!defined(...)) {}`.
define('REQUESTS_SILENCE_PSR0_DEPRECATIONS', true) 

$preload             = WpOrg\Requests\Autoload::getDeprecatedClasses();
$preload['requests'] = '\WpOrg\Requests\Requests'; // Add the base Requests class, just to be safe.

// Preload the class aliases for the Requests 1.x classes to ensure only Requests 2.x classes get loaded.
foreach ($preload as $old => $new) {
    // Make sure we don't get "Class already exists errors" from autoloading chains
    // Think: an `implements` causing an interface to be loaded before we explicitly request it.
    if (class_exists($old) === false && interface_exists($old) === false) {
        Autoload::load($old);
    }
}

@schlessera @alpipego What do you think ? Would that work for you ?

A couple of caveats:

• If WP < 5.2 is still supported, you may run into trouble if PHP < 5.6 is being used.
• There are some changes in Requests 2.0 which projects which expect Requests 1.x and don't use Requests as documented may run into, like stricter input validation, but that shouldn't occur often and IIRC from implementing Requests 2.x in WP, there was only one adjustment which needed to be made, but otherwise, it was fine.
  Also see: https://github.com/WordPress/Requests/blob/develop/CHANGELOG.md and Upgrade to Requests 2.0.0 wordpress-develop#1624

Note that the preloading with the lowercase "old" names is not an issue as class/interface names in PHP are case-insensitive.

[jrfnl]

Oh and once you've tested this and it's okay, we may want to add it as an example in the examples directory ?

[jrfnl]

Some additional notes:

• For use in a WP context, the actual preloading should probably be wrapped in a version check once WP has a release which includes Requests 2.x, as in combination with those WP versions, the preloading shouldn't be needed.
  If the bootstrap does not have access to WP, but the WP files are available, the wp-includes/version.php file could probably be loaded/read (preferably within a function if including) to get the WP version.

Also @costdev: you may find the above an interesting read in the context of the WP Upgrader component.

[alpipego]

Thank you @jrfnl. The code worked for me with minor changes:

require_once 'path/to/Requests/src/Autoload.php';

// Make sure the autoloader is registered first as otherwise you may run into trouble on PHP 8.1.
// See: https://news-web.php.net/php.internals/115549
WpOrg\Requests\Autoload::register();

// Silence deprecations.
// Depending on when the bootstrapping is done, you may need to wrap this in a `if (!defined(...)) {}`.
- define('REQUESTS_SILENCE_PSR0_DEPRECATIONS', true) 
+ define('REQUESTS_SILENCE_PSR0_DEPRECATIONS', true);

- $preload             = WpOrg\Requests\Autoload::getDeprecatedClasses();
+ $preload           = WpOrg\Requests\Autoload::get_deprecated_classes();
$preload['requests'] = '\WpOrg\Requests\Requests'; // Add the base Requests class, just to be safe.

// Preload the class aliases for the Requests 1.x classes to ensure only Requests 2.x classes get loaded.
foreach ($preload as $old => $new) {
    // Make sure we don't get "Class already exists errors" from autoloading chains
    // Think: an `implements` causing an interface to be loaded before we explicitly request it.
    if (class_exists($old) === false && interface_exists($old) === false) {
-        Autoload::load($old);
+       WpOrg\Requests\Autoload::load($old);
    }
}

I have added the new method to the autoloader and added the example to the examples directory. Please see #681.

[jrfnl]

@alpipego Ha, that's what I get for quickly writing out a code sample in a comment instead of a code editor 😂

I'll have a look at the PR. Thanks for testing and writing this up.

[jrfnl]

Pinging @hellofromtonya as a FYI as well.

[jrfnl]

Leaving this open until @schlessera has been able to test the above with WP-CLI.

Alain: please let us know how you get on & close the issue if the changes made in #681 solve this sufficiently.

[jrfnl]

@schlessera Just checking in: have you had a chance to test this with WP-CLI in the mean time ?

[jrfnl]

@schlessera Just checking - has this been tested with WP-CLI by now ?

[schlessera]

This has been resolved through PR wp-cli/wp-cli#5795 as well as its follow-up hotfix wp-cli/wp-cli#5796.