WordPress / Requests

Requests for PHP is a humble HTTP request library. It simplifies how you interact with other sites and takes away all your worries.
https://requests.ryanmccue.info/
Other
3.57k stars 498 forks source link

Dropping Reference (&) From register() method of interface Requests_Auth causing existing dependent libraries incompatible with older version of WordPress #507

Closed nurul-umbhiya closed 3 years ago

nurul-umbhiya commented 3 years ago

Hello, We have an integration with https://github.com/wirecardBrasil/moip-sdk-php which dependents on https://github.com/WordPress/Requests library. From WordPress version 5.8, Reference (&) was removed from interface Requests_Auth register() method https://github.com/WordPress/Requests/blob/43c2e45e164fecf23e2f0e87585af30dca889da0/library/Requests/Auth.php#L32. I can update the Wirecard library to reflect the latest version of WordPress but If I do so, users of my plugin need to update both plugin and WordPress to the latest version. If someone does update to the latest version of WordPress and use old version of our plugin, they'll get fatal error and this goes other ways too.

I'm not sure if I report this as bug or not, but as WordPress 5.8 used latest version of this library, this creates inconsistency with older version of WordPress.

Error I'm getting this is:

PHP Fatal error: Declaration of Moip\Auth\BasicAuth::register(Requests_Hooks &$hooks) must be compatible with Requests_Auth::register(Requests_Hooks $hooks) in [..]/modules/moip/moip-sdk/src/Auth/BasicAuth.php on line 11

Hope I explained myself well.

jrfnl commented 3 years ago

@nurul-umbhiya Thanks for opening this issue and I'm sorry it is causing you trouble.

For the record: the reference was removed in #396 as objects are always passed by reference since PHP 5.0, so this reference was redundant and should never have been in the method signature in the first place as, even when the project was first started, the minimum PHP requirement was PHP 5.2.

I've had a quick think about possible solutions for you:

  1. You could choose to not implement the Requests_Auth interface. This would remove the enforcement of the method signature and as - based on a quick code search verification - the Requests library doesn't actually use the Requests_Auth type declaration anywhere, nor checks for instanceof Requests_Auth anywhere, the integration with Requests should still work as before.
  2. You could choose to ship your plugin with your own dependencies, of which Requests is one. This would allow your code to depend on your preferred version of Requests and for you to manage updates to the integration independently of the WP shipped version. To prevent naming conflicts with the WordPress shipped version, you can use the PHP_Scoper package to prefix everything Requests puts in the global namespace with a prefix.

Would either of these solutions work for you ?

jrfnl commented 3 years ago

@nurul-umbhiya Just checking - did either of my suggestions work for you ?

schlessera commented 3 years ago

@nurul-umbhiya I can only second what @jrfnl has already stated.

The main issue here is that you are relying on an internal implementation of the WordPress HTTP API. This means you'll have to keep up with code changes to these internal Core parts and write compatibility workarounds as needed.

It would have been better if we had removed the reference during a major version updated (i.e. in v2.0), but as you are not pulling in the library as a version-controlled dependency anyway, that would not have changed anything for your case. You are just relying on files that happen to lay around in WP Core without checking their version.

In addition to what @jrfnl has proposed as workarounds, here's one more way of solving this in your code:

  1. Use two separate files with different signatures and conditionally load the correct one. You can see an example of doing that here: https://github.com/wp-cli/wp-cli/pull/5283

With all of the above said, I'm closing this issue, as this is (unfortunately) just part of keeping a tight integration with WordPress Core working across versions.

nurul-umbhiya commented 2 years ago

@jrfnl thanks for your suggestion, I've updated the Wirecard codebase and loaded it from my project manually. @schlessera thank you for the nice suggestion.