Not a bug "Optional parameter before required" in \XF\Repository\UserAlert

[DragonByte Tech]

Affected version

2.0.9

This block:

    public function alertFromUser(
        \XF\Entity\User $receiver, \XF\Entity\User $sender = null,
        $contentType, $contentId, $action, array $extra = []
    )

triggers a warning in phpStorm because the $sender variable is optional, but then followed by a required $contentType variable.

This looks like the sort of thing that would be tightened in a future version of PHP as they slowly move towards getting rid of bad practices from the early days of PHP, so given that people need to pass null to it anyway, we might as well drop the "optionality" of that argument


Fillip

 

[Chris D]

We can't unfortunately; any changes to the argument list for extendable methods will break backwards compatibility; I'm fairly certain that applies to argument optionality too.

I think this also protects against the sender argument being null for some reason; perhaps if it comes from a relation or similar and that has become null for some reason.

However ultimately I think this is designed specifically whereby null is analogous for ”no user” (moderator action alerts, for example) so we’d actually have to lose the argument type here too which would be less clear and could lead to an unexpected other value being passed in.

So all that to say, the argument is already not optional; it must be a user or null and nothing else is acceptable.

If it ever becomes a problem we’ll have to change it, but currently the PhpStorm warning can be ignored.

 

[Sim]

Just FYI - this code is completely valid for the reasons @Chris D mentioned - if $sender could be null and that is an acceptable parameter, you must supply a default parameter of null to stop PHP choking on that parameter.

Without default parameter:

    public function actionTest()
    {
        $receiver = \XF::visitor();
        $sender = null;

        return $this->alertFromUser($receiver, $sender, 'foo', 'bar', 'baz');
    }

    public function alertFromUser(
        \XF\Entity\User $receiver, \XF\Entity\User $sender,
        $contentType, $contentId, $action, array $extra = []
    )
    {
        \XF::dump($sender);
    }

    // output: An exception occurred: [TypeError] Argument 2 passed to ...::alertFromUser() must be an instance of XF\Entity\User, null given

With default parameter:

    public function actionTest()
    {
        $receiver = \XF::visitor();
        $sender = null;

        return $this->alertFromUser($receiver, $sender, 'foo', 'bar', 'baz');
    }

    public function alertFromUser(
        \XF\Entity\User $receiver, \XF\Entity\User $sender = null,
        $contentType, $contentId, $action, array $extra = []
    )
    {
        \XF::dump($sender);
    }

    // output: null

 

[Kirby]

Ideally the signature should be

public function alertFromUser(
    \XF\Entity\User $receiver, $contentType, $contentId, $action,
    \XF\Entity\User $sender = null, array $extra = []
)

but that can't be changed (now) as it would break backwards compatibility

Maybe the method name could be changed in a future version (2.1) and this method could generate E_USER_DEPRECATED and finally be removed way later?

 

[Sim]

I don't think it needs to be changed - the use of default parameters serves two purposes:

1. to avoid needing to supply all parameters to a function
2. to indicate that null is an acceptable value for a type-hinted parameter

In this case, it is the latter use and re-arranging the parameter list serves no valuable purpose other than to make us all feel better about it.

Interestingly, this problem starts to go away as of PHP 7.1, because we get the new nullable types syntax.

Nullable types​

Type declarations for parameters and return values can now be marked as nullable by prefixing the type name with a question mark. This signifies that as well as the specified type, NULL can be passed as an argument, or returned as a value, respectively.​

<?php

function testReturn(): ?string
{
    return 'elePHPant';
}

var_dump(testReturn());

function testReturn(): ?string
{
    return null;
}

var_dump(testReturn());

function test(?string $name)
{
    var_dump($name);
}

test('elePHPant');
test(null);
test();

This new syntax will move us away from using the somewhat confusing default parameters for indicating nullable types and everyone should be happy

 

[S Thomas]

Can't wait to be able to use this. I expect this to land on client machines as early as... 2030. I guess.

 

[Sim]

Can't wait to be able to use this. I expect this to land on client machines as early as... 2030. I guess.

New PHP versions:

• working on your own stand-alone project: YIPPEE - I can make good use of this feature!
• working on libraries to be shared with other people: BOOHOO - I'll never be able to make use of this feature!

All we need is for the XF devs to mandate a minimum of PHP 7.2 for the next version of XF and we'll be good to go

 

[Chris D]

PHP 7.0 isn’t unlikely for 2.2 if the PHP 7.x usage stats we are observing continue upwards.