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

Middleware cannot release SendQueuedMailable again #44123

Closed
dododedodonl opened this issue Sep 14, 2022 · 4 comments · Fixed by #44124
Closed

Middleware cannot release SendQueuedMailable again #44123

dododedodonl opened this issue Sep 14, 2022 · 4 comments · Fixed by #44124

Comments

@dododedodonl
Copy link
Contributor

  • Laravel Version: 9.30.0
  • PHP Version: 8.1.10
  • Database Driver & Version: mariadb 10.3 and 10.9

Description:

It looks like the Illuminate\Queue\InteractsWithQueue trait is missing on Illuminate\Mail\SendQueuedMailable, which results in an PHP error when using middleware like #37568

Steps To Reproduce:

Tested with QUEUE_CONNECTION set to database and sync, and MAIL_MAILER set to log and smtp

  • clone dododedodonl/mail-middleware@39cff6c
  • trigger the mail \Illuminate\Support\Facades\Mail::send(new \App\Mail\RandomMail());
    • this works as expected (the rate limiter is set for 1 per hour)
  • trigger the mail again \Illuminate\Support\Facades\Mail::send(new \App\Mail\RandomMail());
    • rate limiter does what it should
    • it should release the job back onto the queue with a delay, but results into an exception
PHP Error:  Call to undefined method Illuminate\Mail\SendQueuedMailable::release() in vendor/laravel/framework/src/Illuminate/Queue/Middleware/RateLimited.php on line 91
@rodrigopedra
Copy link
Contributor

rodrigopedra commented Sep 14, 2022

I needed this recently, I extended Illuminate\Mail\SendQueuedMailable and added the trait manually.

Then in the mailables I added a custom trait that overrode newQueuedJob() to return my local instance.

I also tweaked how middleware is handled as on the original newQueuedJob() it will run middleware defined on the mailable right after instantiating the job, and I wanted middleware to be ran by the queue as other jobs.

I didn't try a PR at the time as I thought my use-case might be a corner case.

For reference here is:

my custom job "wrapper"
<?php

namespace App\Jobs;

use Illuminate\Mail\SendQueuedMailable;
use Illuminate\Queue\InteractsWithQueue;

class MailableJob extends SendQueuedMailable
{
    use InteractsWithQueue;

    public bool $failOnTimeout = true;

    public function middleware(): array
    {
        // read job middleware from wrapped mailable
        return \array_merge(
            \method_exists($this->mailable, 'middleware') ? $this->mailable->middleware() : [],
            $this->mailable->middleware ?? [],
        );
    }

    public function backoff(): array
    {
        return [60, 105, 120];
    }

    public function retryUntil(): ?\DateTimeInterface
    {
        if (\method_exists($this->mailable, 'retryUntil')) {
            return $this->mailable->retryUntil();
        }

        return null;
    }
}
and the trait I add to my mailables to use my custom job class intead of `SendQueuedMailable`
<?php

namespace App\Jobs;

trait UsesMailableJob
{
    protected function newQueuedJob(): MailableJob
    {
        return new MailableJob($this);
    }
}

Just for completeness, in my use case I needed to use the following job middleware on some mailables:

  • RateLimited: to rate limit mail sending due to comply with mail provider's rate limit across several mailables
  • ThrottlesExceptions: throttle some monitoring emails to not overflow the mail queue

I know there are best tools for monitoring, but for this specific project alerting by email was a requirement.

@ankurk91
Copy link
Contributor

ankurk91 commented Sep 16, 2022

I reported the issue in past

#43571

@driesvints How can we (reporters) improve so that such issues get attention by Laravel team.

@driesvints
Copy link
Member

@ankurk91 like I suggested on the issue, try a support channel first and get confirmation by others that it's an actual bug. Then repost the issue with links to those forum threads.

@ankurk91
Copy link
Contributor

@driesvints Thanks, i would prefer to create a minimal reproduction and share the git repo link.

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 a pull request may close this issue.

4 participants