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 499 forks source link

[PHP 8.4] Fixes for implicit nullability deprecation #865

Closed Ayesh closed 8 months ago

Ayesh commented 8 months ago

Pull Request Type

This is a:

Context

Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations.

See:

Quality assurance

Documentation

For new features:

Art4 commented 8 months ago

Will this require to drop support for PHP 5.6 and 7.0?

Ayesh commented 8 months ago

Ah, I thought this library also requires the same PHP version as WordPress. But yes, you are right if this is merged, it will break on PHP < 7.1.

jrfnl commented 8 months ago

@Ayesh Thanks for the PR.

@Ayesh @Art4 WordPress still has a minimum of PHP 7.0, this package of PHP 5.6, so this change can not go in.

I'd already been looking at this one and have another patch prepared, but hadn't pulled it yet as I wanted to discuss it with @schlessera first.

We will basically need to make a choice between:

  1. Dropping support for PHP < 7.1 and adding the nullable operator. Unlikely that would be acceptable until WP would drop support for PHP < 7.1.
  2. Making the parameter required Iri $origin. This would be a BC break which impacts all calls to the method.
  3. Removing the type $origin = null and validating the parameter (if passed) to be instanceof Iri within the function itself. Still a BC break as the class is not final and the method is not final either, so it could be overloaded from child classes, which would now get a signature mismatch, but a BC-break for which the impact would be smaller as the method is not overloaded that often (I expect).

I'm leaning towards option 3 (which is the patch I prepared locally).

Ayesh commented 8 months ago

Thank you @jrfnl - you are absolutely right making the parameter type nullable was not going for to work on PHP < 7.1. I tried again in the 3rd approach you mentioned. I think that's the best thing we can do apart from bumping minimum requirement to PHP 7.1+.

I tried to recreate error handling, it may not be perfect, but it tries to mimic PHP 5.6 behavior as well.

jrfnl commented 8 months ago

@Ayesh FYI: Alain and me have discussed this ticket and I've updated the PR to bring back the null default value, add the @throws tag, simplify the condition and add the test.

Ayesh commented 8 months ago

Looking great, thank you!

jrfnl commented 8 months ago

Thanks @Ayesh for working on this. The patch is included in today's 2.0.11 release.