Php_codesniffer: PEAR.Functions.ValidDefaultValueshould not flag type hinted methods with a NULL default argument

[SJrX]

Note that when using default arguments, any defaults should be on the right side of any non-default arguments; otherwise, things will not work as expected. Consider the following code snippet:
...
To specify a type declaration, the type name should be added before the parameter name. The declaration can be made to accept NULL values if the default value of the parameter is set to NULL.

http://php.net/manual/en/functions.arguments.php

Unfortunately this leads to = null on a type hinted object having a double meaning as explained in this RFC https://wiki.php.net/rfc/nullable_types

This existing behavior is not changed by this RFC. The new nullable type feature offers a subset of the functionality of = null with both making a parameter nullable but only = null making a parameter optional and have a default value:

Unfortunately the manual is vague and doesn't really make a distinction. But using the PHP 7.1 RFC syntax a method like:

foo(Animal $animal = null, Car $car)

has the semantics of foo(?Animal $animal, Car $car)

That is the ability to not specify the parameter (i.e., use the default) is lost, but you are permitted to pass null objects still. This can be verified using reflection and checking the number of parameters, vs number of required parameters.

I believe that this rule should specifically exclude the case where an argument is type hinted and has a default argument of null.

[gsherwood]

I believe that this rule should specifically exclude the case where an argument is type hinted and has a default argument of null.

I don't understand why any change to the sniff is needed. It's job is to ensure that arguments that are entirely optional in the function call are placed at the end of the function signature. The nullable flag on a parameter doesn't change if it is optional, and so doesn't change its position in the signature.

If I misunderstanding you, would you be able to provide some sample code that the sniff says is invalid but that you think should be valid?

[SJrX]

Using version 2.6.0

<?php

class A {
}

class B { 

public function thisShouldBeAnError($foo = null, $bar)
{
        echo "Hello";
}

public function hasSemanticMeaningShouldNotBeAnError(A $foo = null, $bar)
{
       echo "Good Bye";
}

public function thisShouldNotHighlightEither(A $foo, $bar);
{
       echo "Farewell";
}


}

$a = new A();

$b = new B();

$b->hasSemanticMeaningShouldNotBeAnError(null, "Nothing");

// This call would trigger a PHP error, bceause the type hint does not have a null on it
//$b->thisShouldNotHighlightEither(null, "Nothing"); 

Running with the XML report prints out these two errors:

<error line="8" column="50" source="PEAR.Functions.ValidDefaultValue.NotAtEnd" severity="5" fixable="0">Arguments with default values must be at the end of the argument list</error>
<error line="13" column="69" source="PEAR.Functions.ValidDefaultValue.NotAtEnd" severity="5" fixable="0">Arguments with default values must be at the end of the argument list</error>

The first error makes sense, there is no reason to put that = null there and that should be a style error. In the second case, it does make sense to put = null on the first argument, because it changes the semantics of the method, and specifically it allows null elements to be called. The Sniff should exclude cases where there is a type hint, and the token after the equals is null.

[aik099]

In the second case, it does make sense to put = null on the first argument, because it changes the semantics of the method, and specifically it allows null elements to be called.

And it makes method parameter having default value of null which makes it optional method parameter by definition 😄 (and PHP reflection also thinks it's optional parameter). Probably same logic of dual purpose = null in method parameter declaration applies to this code fragment:

function myName(array $array = null, $x) { }

This sniff only wants all method parameters having default values to be declared after ones, that need value specified.

[SJrX]

I think the purpose of this sniff is because it makes no sense to do something like

function myName($foo = "Hello", $bar)

That default argument is pointless since you always need to specify $bar, you must always specify a value for $foo.

I'm not sure what you meant by "PHP Reflection also thinks it's optional", that isn't true in the case I pointed to above

Add this afterwards (and fix the semicolon on thisShouldNotHighlightEither)

$bClass = new ReflectionClass(get_class($b));

echo "\n\n";
foreach($bClass->getMethods() as $method)
{
    echo "\n" . $method->name;
    echo "\n\tNumber of Parameters: " . $method->getNumberOfParameters();
    echo "\n\tNumber of Required Parameters: " . $method->getNumberOfRequiredParameters();

    foreach($method->getParameters() as $parameter)
    {
        echo "\n\t" . $parameter->getName();
        echo "\n\t\t Position: " . $parameter->getPosition();
        echo "\n\t\t Default Value Available: " . ($parameter->isDefaultValueAvailable() ? "Yes" : "No") ;
        echo "\n\t\t Optional: " . ($parameter->isOptional() ? "Yes" : "No");

    }
}

The output I get is

thisShouldBeAnError
        Number of Parameters: 2
        Number of Required Parameters: 2
        foo
                 Position: 0
                 Default Value Available: Yes
                 Optional: No
        bar
                 Position: 1
                 Default Value Available: No
                 Optional: No
hasSemanticMeaningShouldNotBeAnError
        Number of Parameters: 2
        Number of Required Parameters: 2
        foo
                 Position: 0
                 Default Value Available: Yes
                 Optional: No
        bar
                 Position: 1
                 Default Value Available: No
                 Optional: No
thisShouldNotHighlightEither
        Number of Parameters: 2
        Number of Required Parameters: 2
        foo
                 Position: 0
                 Default Value Available: No
                 Optional: No
        bar
                 Position: 1
                 Default Value Available: No
                 Optional: No

It says that default values are available, but it specifically says they are not optional.

[aik099]

Then I have no idea what ReflectionParameter::isOptional method do, because it always returns false and no documentation exists behind it's internal logic. Probably the ReflectionParameter::isDefaultValueAvailable is the one to be used to detect if default value is present.

[SJrX]

aik099, it doesn't always return false, it returns true in cases like

function hello($foo, $bar = "hello")
{
// Implementation.
}

The fact that the argument is optional gets dropped by PHP. Essentially when PHP sees an parameter without a default argument after, it will make everything previous non-optional. It does not lose the information about the default argument (you need it occasionally when making subtypes with reflection), but it still internally allows type hinted parameters to be passed with null, if null was the default argument.

The point is that in Example 5 of Function Arguments is incorrect, and this check should warn about it. But it does make sense (for instance if you need to generalize an interface), to have a type hinted argument have null specified as the default value, not so that the parameter becomes optional, but so that you can pass null as that argument.

If there is any objections to this related to the fact that using null is sub-optimal anyway, I fully concur, but it isn't relevant to this style check.

[aik099]

Thanks for digging into this.

I think sniff currently assumes that if parameter with default values comes before parameter without default value, then:

• it maybe placed in wrong order
• need to have default value as well

and reports that as an error/warning. Since sniff have no idea which of 2 above mentioned issues happened it can't emulate logic of ReflectionParameter::isOptional method. Does it make sense?

Reflection for both function testMe($param1 = 'x', $param2) and `function testMe($param1 = null, $param2) functions say that $param1 is not optional, because $param2 says so. This suggests that we also shouldn't make any exclusions for = null.

Again I might be missing something here, so let's wait for @gsherwood opinion on this.

[SJrX]

I don't believe that you should make an exclusion for the case when the argument is equal to null. I am saying you should make an exclusion for the case when the argument is equal to null and there is a type hint.

In particular the method: allowsNull() method on ReflectionParameter is what is the litmus test should be, not isOptional().

function myfunction($param) // allowsNull == true
function myfunction($param = null) // allowsNull == true
function myfunction(stdClass $param) // allowsNull == false
function myfunction(stdClass $param = null) // allowsNull == true

And I think the rule is if you are declaring a type hint, and a default value of null, that should be skipped by this style check (or at least have a parameter to allow it to be).

[gsherwood]

Thanks for the detailed explanation. I was looking at the wrong part of this problem.

To summarise:

In PHP, you can't just pass NULL into a function argument if that function argument is type hinted. The only way to achieve that is to add = null to the argument in the function signature. This doesn't technically make the argument optional, but instead just allows the argument to accept a NULL value.

This appears to make less sense when you consider an example with a single argument, as pointed to in the nullable types RFC. For a function with this signature:

function foo_default(Bar $bar = null) {}

You do not need to pass in a value for $bar. The reflection API shows this:

foo_default
    Number of Parameters: 1
    Number of Required Parameters: 0
    bar
         Position: 0
         Default Value Available: Yes
         Optional: Yes

But it makes perfect sense. The reason this is optional is just because there are no required arguments after $bar in the function signature. @SJrX has already explained this in a previous comment.

As this sniff is entirely focused on ensuring optional arguments come at the end of the argument list, I agree that it should make an exception for arguments that are type hinted and have a default value of NULL. These are not optional by default, and so can go anywhere within the function signature.

[gsherwood]

I've fixed up the sniff. Thanks again.

[aik099]

@gsherwood , which function declarations won't be reported as error after the fix:

1. function foo_default(Bar $bar = null) {}
2. function foo_default(Bar $bar = null, $required) {}

?

[gsherwood]

which function declarations won't be reported as error after the fix:

Neither. They are both valid.

[SJrX]

Thank you @gsherwood

[gmponos]

How do you define what is a default value and what is not?

I mean if you think about it in PHP you can't do this

function myName(DateTimeInterface $obj = new DateTime('2017-02-02'), $x) { }

Most probably you will end up with a code like this:

function myName(DateTimeInterface $obj = new DateTime('2017-02-02'), $x) { 
    if ($obj === null) {
        $obj = new DateTime('2017-02-02);
   }
}

So that kinda makes it a default value.

[aik099]

If we look at sniff code, then parameter is considered optional and positioned wrongly when:

• it isn't type hinted and has some default value (including null)
• it is type hinted and it's default value isn't null

[aik099]

And you have an error in your code. You probably wanted to have (DateTimeInterface $obj = null, ...) in 2nd function declaration. That's actually what I'm doing.

[gmponos]

As I said. I can't put a default value in typehinted arguments.

I can't do this

function myName(DateTimeInterface $obj = new DateTime('2017-02-02'), $x) { }

The only way for all PHP guys to set a default value to the above example is setting as null and do an if statement inside their function.

So how do you really define if the null was set there in order to set a default value or not?

[aik099]

As I said. I can't put a default value in typehinted arguments.

This is good, don't you think? That allows to have nice method declaration like so:

function myName(DateTimeInterface $obj = null, $x) { }

This way the developer using the method shouldn't bother retyping new DateTime('2017-02-02') on each call of the method and can just pass null.

[gmponos]

Codesniffer is here to help reviews and fix problems not maintain them.

We are all developers and we all make mistakes. So let's say that a developer had a bad day and writes the following function.

function myFunction(DateTimeInterface $obj = null, $x)
{
    if ($obj === null) {
        $obj = new DateTime('2017-02-02');
    }
    return $obj->format('Y-m-d') . $x;
}

The requirement was that the obj would be optional and have a default value with the current date. The only way for me (as a second developer) to use the function would be:

echo myFunction(null, 'test');

If you think about it.. the code above does not actually make the $obj optional or does it?

But if codesniffer had detected this then the first developer would have fixed the function to this:

function myFunction($x, DateTimeInterface $obj = null)
{
    if ($obj === null) {
        $obj = new DateTime('2017-02-02');
    }
    return $obj->format('Y-m-d') . $x;
}

I would've been able to use the function like this echo myFunction('test');

The only way for the first developer to fulfill the requirement of having a default value and optional with the current date would be to set it as null. So null is kinda a default value sometimes.

If he puts it in as the first argument in the function then the variable $obj still has in a way a default value but it's not optional.

Furthermore, I do understand that having the null as first argument is possible, but I don't understand the reason. And even if an example like this might exist it would be hard for me to accept that it will be a good code example. As I said codesniffer is here to prevent us from writing bad code so I really do not understand why this was accepted.

[aik099]

https://github.com/squizlabs/PHP_CodeSniffer/issues/1092#issuecomment-277051319

Agreeing with you on 100%.

The motivation of @SJrX behind the change was, if I'm not mistaken, to emulate ?ClassName $paramName behavior of PHP 7.1 as ClassName $paramName = null behavior on PHP 5 or 7.0.

From original issue description you can see, that nullable types as sub-set of optional parameters. But after https://github.com/squizlabs/PHP_CodeSniffer/commit/20c46e1f1001317b2aa31a77baffddf9c3c6faff they're made equal, which is totally a BC break.

[SJrX]

@aik099, @gmponos.

PHP does not support operator overloading so sometimes this happens. I do agree this is bad style, and I do agree that CodeSniffers job is to stop you from producing bad code.

This is however a valid bug. That particular sniff is wrong, and should not have flagged that issue. What you both want is a new sniff, that can be optionally enabled.

The original motivation for this rule was presumably because it doesn't make sense, but it does make sense in this case, and so this rule should not flag it. It is bad style, and should be flagged. But you and I should have a choice in it. In my case I do not want that flagged for a number of reasons.

[SJrX]

@gmponos, if you force that rule, you make it so that if I need to change an interface to allow a type hinted argument to be null, I need to break every single client, instead of a small change which is completely backwards compatible. That seems excessive. Even if PHP had method overloading, I would probably have made a new method that doesn't take a null, and then call the existing method with a null.

[gmponos]

Hmmm.. never say never. @SJrX after all there is an example that convinced me.

[aik099]

Then to avoid BC break the allowNullableTypes sniff parameter (defaults to false) should be added. Then if in ruleset the allowNullableTypes is enabled for that sniff, then @SJrX behavior will happen. If allowNullableTypes will be disabled (default), then @gmponos and mine behavior will happen.

[SJrX]

This is not my project, so I have no weight to say it but in my view:

1. This is a code sniffer style rule, which means backwards compatibility is less important.
2. As @gsherwood said the point of this sniff is:

As this sniff is entirely focused on ensuring optional arguments come at the end of the argument list, I agree that it should make an exception for arguments that are type hinted and have a default value of NULL. These are not optional by default, and so can go anywhere within the function signature.

This sniff's purpose the reason that 99% of people enabled it, probably even yourselfs is to prevent pointless defaults to be put in the code. This isn't a pointless default. If you don't want people passing null to type hinting arguments, then don't use it.

Also while I agree that passing null to an argument is bad form, I don't see why it's completely 100% fine for it to be at the end of the argument list.

[gmponos]

@SJrX and @aik099 have in mind that this Sniff is included in the PSR-2.

OK. there might be cases that the null not being at the end of the parameter list is needed but if I understood correct from the asnwers here these arguments are still considered having a default value and should go to the end.

PS: Although the link above is about PSR-12 that it's on draft state the same text about default values exists on the PSR-2 also. PSR-2 and PSR-12

[schnittstabil]

I believe if one targets PHP < 7.0, then (ClassName $paramName = null, $x) is fine. There are often good reasons to choose such an order and =null is the (only good?) way to allow null values. Thus =null does not necessarily mean the author wanted a default value.
Therefore, I believe such a rule shouldn't be enabled by default. But it would be awesome to see a warning, already today.

Of course, when the support for PHP 5.6 and 7.0 ends, that rule can be safely enabled by default.