Properly handle queued mailables and notification.

[ivalkenburg]

Mailables and notifications implementing ShouldQueue were not properly being handled. This fix should inject the tenant id into the wrapper jobs handling mailables and notifications.

To replicate make sure you have queues_are_tenant_aware_by_default set to false and your ShouldQueue mailable/notification implements TenantAware.

[masterix21]

@ivalkenburg i have created a mailable that implements ShouldQueue with:

@component('mail::message')
Welcome to {{ \Spatie\Multitenancy\Models\Tenant::current()->domain }}

Next, from tinker I have executed:

Tenant::first()->makeCurrent();
Mail::to('me@me.com')->queue(new \App\Mail\TenantHello());

Using Redis, the mail is delivered "tenant" aware. What I'm doing wrong that doesn't help me to understand the PR?

[ivalkenburg]

That is probably because you have queues_are_tenant_aware_by_default set to true. Sorry i should've probably added to my initial post.

Setting queues_are_tenant_aware_by_default to false and adding TenantAware to ShouldQueue mailable like is intended will give you error that you're trying to get name from null object because no tenant is set. This happens because the wrapping job that wraps the mailable does not implement TenantAware. This should fix that.

[masterix21]

Sorry @ivalkenburg, but I don't understand why you need a tenant in a job if you say to the package that your jobs are not tenant-aware. Shouldn't be more simple to use queues_are_tenant_aware_by_default to true?

[ivalkenburg]

Sorry i don't understand what you mean. Setting queues_are_tenant_aware_by_default to false and queuing mailable which implements TenantAware will not change the context when the mail is being send.

Also if you wanted to queue a mailable or notification while having no tenant set (being a landlord for example) and queues_are_tenant_aware_by_default enabled it would throw CurrentTenantCouldNotBeDeterminedInTenantAwareJob exception because tenantId was injected with null value. Implementing NotTenantAware would have no effect if put on mailable or notification because they get wrapped in a handler job internally by laravel.

[masterix21]

@ivalkenburg, what do you think about a configurable parser instead of a switch?

For example, you could add in the config it:

'queue_jobs_parser' => [
    SendQueuedMailable::class => fn ($job) => $job->mailable,
    SendQueuedNotifications::class => fn ($job) => $job->notification,
    // ...
],

And next you could change the switch with something like this:

if ($parser = config('multitenancy.queue_jobs_parser.'. get_class($job)) {
    $job = $parser($job);
}

It's an idea, and I'd like to listen to your opinion and what @freekmurze thinks.

[ivalkenburg]

Unfortunately you can not pass functions in configs, it would break config caching. serialize does not serialize functions. I guess if you wanted to do that you would have to define a class/method to do the parsing. But at this point you might as well do it in a method in MakeQueueTenantAwareAction and extend it if you wanted to add functionality.

[masterix21]

Yes, you're right. Sorry for it, but I hate the switch syntax (it's only a bug of mine 🤣)

[freekmurze]

Can you rebase this PR with the current master and let me know if the PR is ready for review?

[ivalkenburg]

@freekmurze squashed and rebased, ready for review.

[freekmurze]

@ivalkenburg thanks for this. Could you also add tests to make sure that this works?

[ivalkenburg]

I'm not seeing a way to test whether or not the mailable/notification and listener are in the correct context as I don't have access to the underlying job from said objects. And testing Tenant::current() is not reliable at all. Regular jobs are in tenant context for the existing tests as well. :( Hence the assert was left out in the tests and opted to inspect the actual payload instead.

[freekmurze]

Could you add an object typehint and string return type.

[ivalkenburg]

Added type hint for object. Sorry i'm not used to using type hints a lot for primitives. Either way, there is no union type for the return type as it can return both a string (FQCN) as well as an object. Hence the reason I used class reflection to check whether or not they implement the necessary interfaces as it can accept a string or object.

[freekmurze]

@masterix21 does this also look good to you?

[masterix21]

@freekmurze, as I previously said, I would have preferred a configurable solution (without callable, but in the style of your tasks logic) instead of extending the class to check with a switch if any new queuable is compatible. Anyway, now this is a bug, and it's better to resolve it: we will take all the time to find a better solution next.

I hope only that @ivalkenburg finds a way to test the code if it exists.

[masterix21]

@freekmurze, do you have any suggestions to test the PR? Or can we merge?

[freekmurze]

Let's merge now and refactor later 👍

[masterix21]

@ivalkenburg, thank you