[PHP 8.4] Fixes for implicit nullability deprecation by Ayesh · Pull Request #865 · WordPress/Requests

Ayesh · 2024-03-15T19:30:04Z

Pull Request Type

I have checked there is no other PR open for the same change.

This is a:

Bug fix
New feature
Documentation improvement
Code quality improvement

Context

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

See:

Quality assurance

This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
I have commented my code, particularly in hard-to-understand areas.
I have added unit tests to accompany this PR.
The (new/existing) tests cover this PR 100%.
I have (manually) tested this code to the best of my abilities.
My code follows the style guidelines of this project.

Documentation

For new features:

I have added a code example showing how to use this feature to the examples directory.
I have added documentation about this feature to the docs directory.
If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

Art4 · 2024-03-15T19:35:56Z

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

Ayesh · 2024-03-15T20:04:11Z

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 · 2024-03-15T22:33:27Z

@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:

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.
Making the parameter required Iri $origin. This would be a BC break which impacts all calls to the method.
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 · 2024-03-16T14:19:57Z

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.

Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations. See: - [RFC](https://wiki.php.net/rfc/deprecate-implicitly-nullable-types) - [PHP 8.4: Implicitly nullable parameter declarations deprecated](https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated) The "fix" for this deprecation is a bit unorthodox, in that the type delcaration is dropped. This makes it effectively a `mixed` type. Inside the function, it mimics the type check and throws an exception (PHP 7.0+) or triggers an error.

jrfnl

@Ayesh Thank you for making those updates.

I've left some feedback in line for you to consider.

jrfnl · 2024-03-16T15:22:10Z

src/Cookie.php

+		// Mimic a \WpOrg\Requests\Iri|null type-check for $origin.
+		// Change to a type declaration when PHP >= 7.1 is the base requirement.
+		if ($origin !== null && !$origin instanceof \WpOrg\Requests\Iri) {
+			if (\PHP_VERSION_ID >= 70000) {
+				throw new \TypeError(
+					'WpOrg\Requests\Cookie::parse_from_headers(): Argument #2 ($origin) must be of type WpOrg\Requests\Iri, ' . gettype($origin) . ' given'
+				);
+			}
+			trigger_error(
+				'Argument 2 passed to WpOrg\Requests\Cookie::parse_from_headers() must be an instance of WpOrg\Requests\Iri, ' . gettype($origin) . ' given',
+				E_USER_ERROR
+			);
+			exit();
+		}


I think this can be simplified by using the WpOrg\Requests\Exception\InvalidArgument exception, which we're also using everywhere else we're doing input validation in-function in Requests:

if (!empty($origin) && !($origin instanceof Iri)) { throw InvalidArgument::create(2, '$origin', Iri::class, gettype($origin)); }

Also take note of the parentheses around the instanceof. These are necessary as otherwise the precedence order will cause this code to be read as (!$origin) instanceof Iri, which is effectively true|false instanceof Iri, which would always be false.

The function will now also need to get a @throws tag.

As a last point, I wonder if this check should be moved to below the $cookie_headers check ? as that can function just fine without the $origin being validated.

Oh and before I forget: this change will also need a test to cover it. Basically a test passing a non-object and one passing a non-Iri object to the method to make sure those get rejected correctly. The WpOrg\Requests\Tests\TypeProviderHelper can be used to set up the data provider for this test to make it comprehensive.

jrfnl · 2024-03-25T09:31:23Z

@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 · 2024-03-25T09:38:16Z

Looking great, thank you!

Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

jrfnl · 2024-03-25T12:23:55Z

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

jrfnl reviewed Mar 16, 2024

View reviewed changes

Cookie::parse_from_headers(): update to improve PHP 8.4 deprecation fix

e0dba7a

jrfnl mentioned this pull request Mar 25, 2024

Release checklist for 2.0.11 #858

Closed

7 tasks

jrfnl added this to the 2.0.11 milestone Mar 25, 2024

jrfnl added Component: Core Type: bug labels Mar 25, 2024

schlessera approved these changes Mar 25, 2024

View reviewed changes

schlessera merged commit f189e19 into WordPress:develop Mar 25, 2024

jrfnl added a commit that referenced this pull request Mar 25, 2024

[PHP 8.4] Fixes for implicit nullability deprecation (#865)

984a4ae

Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

jrfnl added a commit that referenced this pull request Mar 25, 2024

[PHP 8.4] Fixes for implicit nullability deprecation (#865)

ca998c7

Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

jrfnl added a commit that referenced this pull request Mar 25, 2024

[PHP 8.4] Fixes for implicit nullability deprecation (#865)

88672cc

Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PHP 8.4] Fixes for implicit nullability deprecation#865

[PHP 8.4] Fixes for implicit nullability deprecation#865
schlessera merged 2 commits intoWordPress:developfrom
Ayesh:php84/nullability

Ayesh commented Mar 15, 2024 •

edited

Loading

Uh oh!

Art4 commented Mar 15, 2024

Uh oh!

Ayesh commented Mar 15, 2024

Uh oh!

jrfnl commented Mar 15, 2024

Uh oh!

Ayesh commented Mar 16, 2024

Uh oh!

jrfnl left a comment

Uh oh!

jrfnl Mar 16, 2024

Uh oh!

jrfnl Mar 16, 2024 •

edited

Loading

Uh oh!

jrfnl commented Mar 25, 2024

Uh oh!

Ayesh commented Mar 25, 2024

Uh oh!

jrfnl commented Mar 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Ayesh commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Type

Context

Quality assurance

Documentation

Uh oh!

Art4 commented Mar 15, 2024

Uh oh!

Ayesh commented Mar 15, 2024

Uh oh!

jrfnl commented Mar 15, 2024

Uh oh!

Ayesh commented Mar 16, 2024

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

jrfnl Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl commented Mar 25, 2024

Uh oh!

Ayesh commented Mar 25, 2024

Uh oh!

jrfnl commented Mar 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ayesh commented Mar 15, 2024 •

edited

Loading

jrfnl Mar 16, 2024 •

edited

Loading