Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.8.0] Requests 2.0 is a breaking change, causing fatal errors with Composer projects. #5795

Closed
2 tasks done
justlevine opened this issue Jun 2, 2023 · 18 comments
Closed
2 tasks done

Comments

@justlevine
Copy link

justlevine commented Jun 2, 2023

Bug Report

Describe the current, buggy behavior

V2.8.0 upgraded rmccue/requests to the (breaking) v2.0. However, since 2.8.0 is semantically nonbreaking, any other PHP deps using the older version rmmccue/requests alongside wp-cli v2.x are now broken.

The easiest replication case is simply installing "wp-cli": ">2.7.0" as a dev dependency on a Composer-based WordPress install < 6.2 , and you should get a WSOD (since it installs wp-cli@2.8.0 and therefore requests@2.0 to the same namespace used by WP).

To illustrate the breadth of the issue effect on the ecosystem, installing wp-cli-bundle at a fixed 2.7.1 will cause the same issue (since wp-cli@2.8.0 is meets the SemVer requirements). This holds true if you use tooling like wpbrowser,phpstan-wordpress, or something that relies on 'wordpress-stubs' since they all currently use Requests 1.x internally., but install the latest semantically-compliant version of wp-cli).

(If you have a local plugin or theme, just delete and reinstall your composer deps from your existing lock file, and you'll see those scripts will throw fatal errors).

Temporary workaround is to explicitly set wp-cli to v2.7.x` as a local dependency, so any packages that are expecting a SemVer compatible release get locked to the lower version).

Describe how other contributors can replicate this bug

  1. Create a new WordPress install with composer. Set the WordPress Version to <6.2, and wp-cli-bundle to 2.7.x.
    E.g.
{
  "name": "example/wordpress",
  "type": "project",
  "extra": {
    "wordpress-install-dir": "wp",
    "installer-paths": {
      "content/plugins/{$name}/": [
        "type:wordpress-plugin"
      ],
      "content/themes/{$name}/": [
        "type:wordpress-theme"
      ]
    }
  },
  "repositories": [
    {
      "type": "composer",
      "url": "https://wpackagist.org"
    }
  ],
  "require": {
    "johnpbloch/wordpress": "6.1"
  },
  "require-dev": {
    "wp-cli/wp-cli-bundle": "~2.7.1"
  }
}
  1. Confirm that wp-cli/wp-cli v2.8.0 was installed (and not 2.7.x), and that rmccue/requests is at v2.x (and not v1.8.x)
  2. Visit the WP admin, and see a WSOD with a similar PHP error due to requests 2.0 being included in the autoloader,.
PHP Fatal error:  Uncaught TypeError: Argument 1 passed to WP_HTTP_Requests_Response::__construct() must be an instance of Requests_Response, instance of WpOrg\Requests\Response given, called in /path/to/wordpress/wp-includes/class-wp-http.php on line 397 and defined in /path/to/wordpress/wp-includes/class-wp-http-requests-response.php:42

Note: wp-cli-bundle is used as a proxy for any composer dependency that doesnt have wp-cli set to a fixed version before 6.1

Describe what you expect as the correct outcome
It seems to me a couple things are going wrong here:

  1. wp-cli 2.8.0 should probably not be throwing an error when used on WordPress < 6.2, even if thats WP install is using Composer. I don't think thats really doable without resorting to something equally breaking like namespacing dependencies, which brings me to:
  2. wp-cli 2.8.0 (and therefore rmccue/requests) should not be an acceptable Composer version on projects that rely on rmccue/requests@1.x.

Let us know what environment you are running this on

PHP 8.0.26 (wsl2 + devilbox + ubuntu 20.04). Im filling this at 3AM from my mobile device so ill fill this in later.

Provide a possible solution

TBH I think the real solution is to re-release v2.8.0 as v3.0.0, but I doubt thats gonna fly. Barring that however, all projects that depend on wp-cli (including wp-cli-bundle) will need to lock themselves to wp-cli 2.7.x or upgrade their usage of rmccue/requests to v2.x (which to be clear means that v2.8.0 will no longer be compatible with Composer-based WordPress < 6.2)

Provide additional context/screenshots

As mentioned, im AFK, but here's a real world example affecting Codeception tests in wpgraphql-acf.

@syhc
Copy link

syhc commented Jun 2, 2023

I can also confirm this issue. It is breaking CI for my team.

Steps to reproduce

I am less concern with the version numbering; since WP does not always follows semver. However, I am mostly disappointed:

  1. that this change is not backwards compatible for WP < 6.2 (not everybody can update to latest WP immediately)
  2. previous .phar files in github are not locked to the version specify (have to change approach installing wp cli)
  3. wp cli upgrade command does not have a downgrade ability that wp core upgrade does.

Issue causing the problem:
#5774 -> #5623

@schlessera
Copy link
Member

Thanks for the report, @justlevine .
Requests v2 should not have been a breaking change in the first place. If it is in this scenario, then the way we're pulling it in does not work well enough for this use case, and we need to fix this. I'll work on a 2.8.1 hotfix release to solve this issue.

@schlessera
Copy link
Member

One thing I'd like to note here:

Do NOT pull in wp-cli/wp-cli-bundle when using WordPress with a Composer stack.

The wp-cli/wp-cli-bundle represents the PHAR file build tooling, and therefore comes with a lot of stuff you wouldn't need in a Composer stack. Amongst others, it includes the wp-cli/package-command repository, which is WP-CLI's built-in package manager and is basically an embedded version of Composer.

You don't need any of this in a Composer stack, and this only opens up a can of worms regarding dependencies.

Instead, pull in the individual command packages that you need for your particular project, like wp-cli/core-command, wp-cli/entity-command`, etc... Reduce the dependencies to the minimum that is needed. The framework itself will be pulled in automatically by any command packages, so these alone are enough to make it work.

@justlevine
Copy link
Author

justlevine commented Jun 2, 2023

One thing I'd like to note here:

Do NOT pull in wp-cli/wp-cli-bundle when using WordPress with a Composer stack.

Thanks @schlessera - and understood. I chose to use wp-cli-bundle+ WPonComposer as an example because it's easier to replicate than scaffolding a theme/plugin and has the added benefit of being only an internally-controlled library instead of a third-party one, and by extension the recommended way to include wp-cliin a Composer project. 😇
(My gut tells me that the issue @syhc is experiencing relates to their have a plugin that use WP Remote Request prerequisite, and that those plugins are using old namespaces/classes instead of a b/c core method like wp_remote_request() )

@johnbillion
Copy link
Contributor

johnbillion commented Jun 2, 2023

This isn't specific to WP < 6.2. I'm seeing a fatal error caused because the \WpOrg\Requests\Autoload class is trying to be redeclared when it already exists in the php-stubs/wordpress-stubs package which is used by szepeviktor/phpstan-wordpress:

phpstan analyze
Note: Using configuration file phpstan.neon.dist.
Fatal error: Cannot declare class WpOrg\Requests\Autoload, because the name is already in use in vendor/php-stubs/wordpress-stubs/wordpress-stubs.php on line 18381

TBD: Is the \WpOrg\Requests\Autoload class unconditionally loaded by WP-CLI?

Example composer config:

{
	"require-dev": {
		"wp-cli/wp-cli": "^2",
		"szepeviktor/phpstan-wordpress": "^1"
	}
}

schlessera added a commit to wp-cli/wp-cli-bundle that referenced this issue Jun 2, 2023
@schlessera
Copy link
Member

@justlevine I'm trying to reproduce this issue within a test. However, so far, my testing does not reveal any issue.
Can you have a look at https://github.com/wp-cli/wp-cli-bundle/pull/552/files to verify I'm on the right track?

@justlevine
Copy link
Author

@justlevine I'm trying to reproduce this issue within a test. However, so far, my testing does not reveal any issue. Can you have a look at https://github.com/wp-cli/wp-cli-bundle/pull/552/files to verify I'm on the right track?

I'm not terribly familiar with the .feature syntax or the internals of wp-cli tbh, but it looks like you're testing whether wp-cli is working and not whether any external use of Requests continues to work with wp-cli installed.

A passing test case would be calling code that uses the old v1.x syntax. I can do a stack trace on fatal error to tell you what wp-core method is being called on a real-life visit to the wp-admin dashboard, in case there's a discrepancy between that and a load over cli. One sec:

@johnbillion
Copy link
Contributor

@justlevine Does your WordPress project contain a call to vendor/autoload.php? For example in your wp-config.php file or in a mu-plugin? I think that might be what's missing from your test @schlessera

@schlessera
Copy link
Member

That was it, @johnbillion . Of course we need to trigger the Composer autoloader if we want to test for conflicts with autoloading. 😜

@justlevine
Copy link
Author

great catch @johnbillion! I abstracted my example from a real-world bedrock site, must not have fully cleared the old one out before I tested the replication.

@syhc
Copy link

syhc commented Jun 2, 2023

I forgot that I need to also version lock it on my composer, did not realize one of the packages depends on wp-cli. (Yes one of the mu plugins in my project calls vendor/autoload.php)

Locking wp-cli to 2.7.1 on both composer and the phar works.

@jasonbahl
Copy link

I experienced the same as @johnbillion here #5795 (comment)

and locking wp-cli to 2.7.1 fixes it

@schlessera
Copy link
Member

schlessera commented Jun 2, 2023

Ok, I found the culprit. The Requests v2 library has a static file inclusion in Composer for the compatibility layer. When using WP-CLI with Composer, this compatibility layer gets put in front of the Requests library bundled with Core.

The WP-CLI code cannot even prevent this, this all happens before any of WP-CLI's code is ever hit.

I'm now looking into including the Requests library with WP-CLI in such a way that the autoloader is not being added. We're manually autoloading Requests anyway, it's just a matter of somehow telling Composer to not include that package's autoloader.

Image 2023-06-02 at 9 22 45 PM

jasonbahl added a commit to jasonbahl/wp-graphql that referenced this issue Jun 2, 2023
- remove unused "use" statements
- add test
@BrianHenryIE
Copy link
Member

A quickfix seems to be to add:

  "provide": {
    "rmccue/requests": "*"
  }

to your own composer.json. This will prevent Composer from downloading Requests into vendor but WP CLI can still use the WordPress provided one. (only briefly tested, but PhpStan and wp plugin install both worked for me)

@schlessera
Copy link
Member

Hey everyone,
I think this is now fixed in dev-main. Can you all test to see if the error is gone for you?

@titouanmathis
Copy link

Thanks for the fix @schlessera, installing dev-main fixed the issue with PHPStan for me.

@johnbillion
Copy link
Contributor

Same here, PHPStan testing is working well again 👍

@justlevine
Copy link
Author

Closing this out, thanks for the quick work!

https://github.com/wp-cli/wp-cli/releases/tag/v2.8.1

lipemat added a commit to lipemat/wp-cli-bundle that referenced this issue Jun 5, 2023
* upstream/main:
  Unlock framework version
  Add executable bit to DEB build version
  Update Composer lock file
  Update Composer lock file
  Adapt tests for PHP 8.1+
  Add minimum stability dev
  Adapt test to pull dev-main
  Update make-phar file for requests path change
  Update Composer lock file
  Add debug output to test
  Update wp-cli/search-replace-command to latest (wp-cli#554)
  Load Composer autoloader
  Add test to trigger issue wp-cli/wp-cli#5795
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants