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

Pass queue from Mailable to SendQueuedMailable job #47612

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

Tarpsvo
Copy link
Contributor

@Tarpsvo Tarpsvo commented Jun 29, 2023

Fixes issue where the unserialized job has the wrong queue. This occurs especially when dealing with Cloud Tasks, where we need the correct queue after unserializing the job to fetch or update the status of the task from the server.

Currently, after unserializing, the queue of the job is incorrect, but the $mailable inside the job has the correct queue set to it.

In the screenshot provided below, I have illustrated how the serialized job has a mailable with the correct queue inside it. But when we do getQueue() on the job itself, it will always fall back to the default queue instead of the one for the mailable.

Interestingly enough, when the task is sent to the queue driver, it ends up in the correct queue. The issue arises when the queue driver receives the task and tries to gets it queue using ->getQueue().

image

Fixes issue where the unserialized job has the wrong queue.
@taylorotwell
Copy link
Member

So this documentation does not work?

https://laravel.com/docs/10.x/mail#pushing-to-specific-queues

@Tarpsvo
Copy link
Contributor Author

Tarpsvo commented Jun 29, 2023

The usecase I'm referring to is this:
https://laravel.com/docs/10.x/mail#queueing-by-default

My Mailables extend from a base class which implements ShouldQueue and uses the Queueable and SerializesModels traits. In its __construct, I set their ->onQueue() to a dedicated mailing queue.

It does push the job to the correct queue. However, when the driver receives it and deserializes it, the SendQueuedMailable job will have an undefined queue (see screenshot in original post), causing the driver to think it is running on the default queue. This PR suggests that when deserializing the job, it should also re-apply the queue from the $this->mailable class within.

All other jobs work perfectly fine in my Google Cloud Tasks driver, but the SendQueuedMailable is the only one misbehaving.

Another option would be to look into the serialization of SendQueuedMailable job and ensure that it always applies the queue from $this->mailable to itself before serializing, but that seemed more complicated.

I hope this clarifies things a bit more.

@taylorotwell
Copy link
Member

Interesting - why did you set queue only and not connection?

@Tarpsvo
Copy link
Contributor Author

Tarpsvo commented Jun 29, 2023

That does make sense, my usecase was only limited my default connection. Updated the PR to include connection as well.

@taylorotwell taylorotwell merged commit a88e2d3 into laravel:10.x Jun 30, 2023
12 of 16 checks passed
taylorotwell added a commit that referenced this pull request Jun 30, 2023
* Add sub-minute scheduling

* formatting

* Allow sub-minute events to conditionally run throughout the minute

* Fix test failure caused by new mutex cache

* Formatting

* Pass queue from Mailable to SendQueuedMailable job (#47612)

* Pass queue from Mailable to SendQueuedMailable job

Fixes issue where the unserialized job has the wrong queue.

* Pass connection from Mailable to SendQueuedMailable job

* Fix property mismatches in SendQueuedMailable

* order

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>

* Add sub-minute scheduling

* formatting

* Allow sub-minute events to conditionally run throughout the minute

* Fix test failure caused by new mutex cache

* Formatting

* bail early

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
Co-authored-by: Tarvo R <TarvoReinpalu@gmail.com>
@Tarpsvo Tarpsvo deleted the patch-1 branch July 3, 2023 06:39
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.

None yet

2 participants