[PHP 8.4] Fixes for implicit nullability deprecation

[Ayesh]

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:

• RFC
• PHP 8.4: Implicitly nullable parameter declarations deprecated

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]

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

[Ayesh]

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]

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

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]

@Ayesh Thank you for making those updates.

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

[jrfnl]

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.

[jrfnl]

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]

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

Looking great, thank you!

[jrfnl]

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