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

[5.4] Always put Mailable on queue if ShouldQueue contract exists #18144

Merged
merged 2 commits into from
Feb 28, 2017
Merged

[5.4] Always put Mailable on queue if ShouldQueue contract exists #18144

merged 2 commits into from
Feb 28, 2017

Conversation

KennedyTedesco
Copy link
Contributor

Documentation says:

If you have mailable classes that you want to always be queued, you may implement the ShouldQueue contract on the class. Now, even if you call the send method when mailing, the mailable will still be queued.

With this change, even if I use the send() method, will be queued, because the ShouldQueue interface was implemented:

class OrderShipped extends Mailable implements ShouldQueue
{
    use Queueable, SerializesModels;

    // todo
}

Then:

Mail::send(new OrderShipped()); // Will be queued
Mail::queue(new OrderShipped()); // Will be queued

@taylorotwell Is this makes sense for you?

Docs says:

> If you have mailable classes that you want to always be queued, you
may implement the `ShouldQueue` contract on the class. Now, even if you
call the send method when mailing, the mailable will still be queued.

With this change, even if I use the `send()` method, will be queued:

```php
class OrderShipped extends Mailable implements ShouldQueue
{
    use Queueable, SerializesModels;

    // todo
}
```

Then:

```php
Mail::send(new OrderShipped()); // Will be queued
Mail::queue(new OrderShipped()); // Will be queued
```
@tomcoonen
Copy link

@taylorotwell @KennedyTedesco Since you're putting a 'SendQueuedMailable' on the queue, which does

$mailer->send($this->mailable);

you're creating an endless loop that doesn't send anything..

@KennedyTedesco
Copy link
Contributor Author

KennedyTedesco commented Feb 28, 2017

@tomcoonen Have you tested? How can I reproduce this? Because I have tested on my app Mail::send(new OrderShipped()); and all worked fine.

@tomcoonen
Copy link

@KennedyTedesco I have tested this, keeps queuing.
I simply added your commit to \Illuminate\Mail\Mailer locally to test:

    public function send($view, array $data = [], $callback = null)
    {
        if ($view instanceof MailableContract) {
            if ($view instanceof ShouldQueue) {
                return $view->queue($this->queue);
            }

            return $view->send($this);
        }

@KennedyTedesco
Copy link
Contributor Author

@tomcoonen Got it. So, it's all good or we have any issue?

@tomcoonen
Copy link

@KennedyTedesco No good. Like I said, it keeps queuing without sending.

@KennedyTedesco
Copy link
Contributor Author

@tomcoonen Now I see! I'll send a PR to fix this ASAP.

taylorotwell pushed a commit that referenced this pull request Feb 28, 2017
Refs: #18144

This fixes the infinite recursion the was happening. I have tested on
my local machine and looks good.

@tomcoonen Can you have a look at this?
symfony-splitter pushed a commit to illuminate/mail that referenced this pull request Feb 28, 2017
Refs: laravel/framework#18144

This fixes the infinite recursion the was happening. I have tested on
my local machine and looks good.

@tomcoonen Can you have a look at this?
taylorotwell pushed a commit to illuminate/mail that referenced this pull request Sep 17, 2018
Refs: laravel/framework#18144

This fixes the infinite recursion the was happening. I have tested on
my local machine and looks good.

@tomcoonen Can you have a look at this?
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