`PEAR.Functions.ValidDefaultValue`: doesn't catch PHP 8.1 deprecation

[davidrans]

Describe the bug
This sniff ignores optional parameters with a default value of null if they have a typehint, so it doesn't catch an issue that throws an exception in PHP 8.1

If I comment out the following lines from the sniff it works:

PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/ValidDefaultValueSniff.php

Lines 58 to 63 in ed8e00d

	// Check if the arg is type hinted and using NULL for the default.
	// This does not make the argument optional - it just allows NULL
	// to be passed in.
	if ($param['type_hint'] !== '' && strtolower($param['default']) === 'null') {
	$defaultFound = false;
	}

Code sample
This function should throw a PHPCS error, but doesn't:

public function apply(callable $errorCallback = null,  bool $showProgress): bool {                                 
   ...
}

To reproduce
Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs test.php ...
3. See NO error message displayed
4. But PHP throws the following deprecation warning:

 Deprecated: Optional parameter $errorCallback declared before required parameter $showProgress is implicitly treated as a required parameter

Expected behavior
I would expect the ValidDefaultValue.NotAtEnd sniff to catch this error.

Versions (please complete the following information):

• OS: [e.g., Windows 10, MacOS 10.15]
• PHP: [8.0, 8.1]
• PHPCS: 3.7.2
• Standard: Custom

[jrfnl]

Eh... might just be me, but that code does NOT throw a deprecation in PHP 8.x.

It only does so when you remove the type declaration: https://3v4l.org/cQjUj

[davidrans]

Hmm, I'm not able to reproduce it in that environment either, though I was able to confirm that only passing the second param is an error in PHP 8.1 (and not in PHP 8.0): https://3v4l.org/dLoTl#v8.1.17

[jrfnl]

@davidrans I'm not sure what action is expected here. Can this issue be closed ?

[davidrans]

I believe the code I referenced in my issue is incorrect:

PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/ValidDefaultValueSniff.php

Lines 58 to 63 in ed8e00d

	// Check if the arg is type hinted and using NULL for the default.
	// This does not make the argument optional - it just allows NULL
	// to be passed in.
	if ($param['type_hint'] !== '' && strtolower($param['default']) === 'null') {
	$defaultFound = false;
	}

A type-hinted parameter with a default value of NULL should be treated as an optional parameter.

[jrfnl]

A type-hinted parameter with a default value of NULL should be treated as an optional parameter.

Only when the type is not a Class/Interface etc name.

Parameters with Class/Interface name type declarations are made nullable by the null default. It doesn't necessarily make them optional.

As of PHP 8.0.0, declaring mandatory arguments after optional arguments is deprecated. This can generally be resolved by dropping the default value, since it will never be used. One exception to this rule are arguments of the form Type $param = null, where the null default makes the type implicitly nullable. This usage remains allowed, though it is recommended to use an explicit nullable type instead.

Ref: https://www.php.net/manual/en/functions.arguments.php (above example #10)

[davidrans]

Ah, so maybe this check just needs to get more sophisticated about checking the kind of typehint?

And maybe augmented with a related check that enforces the recommended nullable type syntax?