Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Allow for named arguments via dispatchable trait #38066

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

sleicester
Copy link
Contributor

I've had a situation where it would be nice to use named arguments in dispatching a job to the queue as it allows me to leave out arguments:

RegisterResponse::dispatch(
    uuid: $uuid,
    question: $request->question,
    answer: $request->answer,
    referer: $request->input('referer', $request->headers->get('referer'))
);

Unfortunately this will now work as dispatch, dispatchSync and dispatchAfterRequest don't receive any arguments.

Obviously it's easy enough to go about this a different way:

dispatch(new RegisterResponse(
    question: $request->question,
    answer: $request->answer,
));

Are there any consequences to these simple changes that I'm not foreseeing?

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Jul 19, 2021

I don't think we should actually support named arguments, here. Laravel doesn't support named arguments, so it seems wrong to allow it here. If anything, we could throw an exception saying it's not supported.

@sleicester
Copy link
Contributor Author

Thanks @GrahamCampbell, I did think it could open a can of worms as there are more likely other places that aren't so simple to change, and probably best dealt with when the minimum PHP version is bumped to 8.

Although, if Laravel would want to support named arguments in the future, then perhaps it's good to start looking at small non-breaking changes now?

@GrahamCampbell
Copy link
Member

I can't see Laravel ever wanting to support them, tbh.

@lassemettovaara
Copy link
Contributor

lassemettovaara commented Jul 20, 2021

I don't think we should actually support named arguments, here. Laravel doesn't support named arguments, so it seems wrong to allow it here. If anything, we could throw an exception saying it's not supported.

It's certainly fine for Laravel not to actively support named arguments, but to me this feels like a weird place to explicitly disallow it since it's just proxying user defined arguments to user defined methods.

Also: events sometimes accept named arguments, like:

use Illuminate\Foundation\Events\Dispatchable;

class Foo
{
    use Dispatchable;

    public function __construct(
        public string $foo,
        public bool $bar,
    ) {
        var_dump($this->foo, $this->bar);
    }
}

Foo::dispatchIf(true, foo: 'some string', bar: false);
Foo::dispatchUnless(false, bar: true, foo: 'some other string');

This already works fine, since dispatchIf and dispatchUnless use variadics instead of func_get_args.

Edit: the same obviously goes for jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants