Skip to content

[5.4] Mailables that defined a delay property will honor it#20717

Merged
taylorotwell merged 1 commit into
laravel:5.4from
ipalaus:5.4
Aug 24, 2017
Merged

[5.4] Mailables that defined a delay property will honor it#20717
taylorotwell merged 1 commit into
laravel:5.4from
ipalaus:5.4

Conversation

@ipalaus

@ipalaus ipalaus commented Aug 24, 2017

Copy link
Copy Markdown
Contributor

This commit allows Mailables implementing ShouldQueue to define the $delay property in the Mailable class. This will allow to run Mail::queue(new ExampleMailable)); and honor the delay.

I know you can just use the Mail::later() that will respect the defined value (or assigned with $mailable->delay($delay)) but if we're already implementing the ShouldQueue contract and even the generated Mailable class with the command makes use of the Queueable I don't see a reason why you should be forced to use later().

This also allows to add a delay to any mailable class without having to specifically find where exactly we're sending the email.

@ipalaus ipalaus changed the title Mailables that defined a delay property will honor it [5.4] Mailables that defined a delay property will honor it Aug 24, 2017
@themsaid

Copy link
Copy Markdown
Member

If we do this I think we'll have to allow defining delay in jobs and notifications as well, but imo delaying a queued task is a decision to be taken while dispatching since a single task could be delayed in part of the code base and dispatched instantly in another.

@ipalaus

ipalaus commented Aug 24, 2017

Copy link
Copy Markdown
Contributor Author

@themsaid this is already happening when you dispatch jobs afaik:

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Bus/Dispatcher.php#L172-L174
https://github.com/laravel/framework/blob/5.4/src/Illuminate/Bus/Dispatcher.php#L180-L182

and I would expect a class implementing ShouldQueue to behave as the Job did, that's actually what I expected :)

This is kinda related to #18144 #18160 where Mail::send() would also respect the implementation of ShouldQueue.

@themsaid

Copy link
Copy Markdown
Member

Yes but it wasn't meant to be used from inside the class, was never documented, it's set when you call delay() on the command.

I don't see a problem merging this PR, just wanted to share my opinion :). @taylorotwell will review it and see if he wants to merge

@taylorotwell taylorotwell merged commit e6da9a9 into laravel:5.4 Aug 24, 2017
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.

3 participants