tenancy / multi-tenant

Run multiple websites using the same Laravel installation while keeping tenant specific data separated for fully independent multi-domain setups, previously github.com/hyn/multi-tenant
https://tenancy.dev
MIT License
2.56k stars 393 forks source link

TenantAwareJob Configuration override #787

Open netcom opened 5 years ago

netcom commented 5 years ago

This is tested with hyn/multi-tenant 5.2 I think is worth looking into this issue on latest version. If someone can confirm this is resolved on latest version I would much appreciate that.

I have multiple tenants with seperate databases, one of tenants have mail.php settings override.


Send queue mail job on tenant with mail setting override. Sends correctly. When I queue mail on tenant without mail.php override it uses config from overriden tenant.

Config is not overriden per tenant, it loads config first time and uses same for all other jobs.

TenantAwareJob should be aware of tenant config overrides.


Information

bkintanar commented 5 years ago

@netcom I can confirm that this is a bug.

Config overriding mail.php in tenants directory works only if not using queues.

Testing with 5.4.4 and using Horizon for sending queues and added config override to one tenant. Sending using queue will just read the config/ and not the override.

Sending using normal send reads the override just fine.

bkintanar commented 5 years ago

Steps to reproduce

  1. Setup at least 2 tenants.
  2. Copy the app/config/mail.php to storage/app/tenancy/tenants/{uuid}/config/mail.php.
  3. Customize the mail.php in step #2.
  4. Test Mail::to()->send() on both tenants. This should work as intended, 1st tenant to use the app/config/mail.php and the 2nd should use the one you've overridden.
  5. Test Mail::to()->queue() on both tenants. This is where the problem lies. It only picks up the app/config/mail.php on both tenants and doesn't use the overridden one for the 2nd tenant as intended.

@netcom let me know if I miss anything. Thanks!

Plytas commented 5 years ago

After further investigation it seems that config override does work. When a tenant is identified it also overrides the config values.

This issue lies with how Mail service is set up on laravel. It's being registered as a singleton the first time it is accessed (Laravel source) and with queues that happens before the tenant is identified.

For now there's a manual solution. Create new SwiftMailer instance on the mailable's send function https://stackoverflow.com/a/46135925/9840518

We'll try to look into possible solutions on our side:

  1. Try to find a way to re-boot Mail instance so it pick up new config values.
  2. Hook into queue processing earlier and identify the tenant before Mail instance gets created.
drbyte commented 5 years ago

@Plytas could you just remove Illuminate\Mail\MailServiceProvider::class, from the providers array in /config/app.php and then manually register it later in another serviceprovider where it makes sense for your workflow? I agree this is a bit clunky (ie: less "clean" than just "add this package and configure it" for a new Laravel app), but it gives you complete control.

Plytas commented 5 years ago

Not sure that would work. I'm guessing Mail service is being instantiated somewhere before JobProcessing event is called.

netcom commented 5 years ago

I ended up using @Plytas suggestion and override the send() method and implemented Swift_Mailer that way. Also this package could easily add Trait (example TenantAwareMailJob) or something like that which overrides send() method and loads connection from overriden config using website_id as reference to tenant disk.

luceos commented 5 years ago

@Plytas if I'm right then the easiest solution is to flush the singleton from the container using forgetInstance? We do something comparable with the filesystem disk as well.

Plytas commented 5 years ago

Thanks! Can't find any reference to that, but I'll do some research.

luceos commented 5 years ago

One consequence though is that the instance has already been resolved before we are able to flush it. I can imagine this is likely for a queued mailable, but I'm not sure about that. A test should prove the validity of my assumption (just flush the instance from the container and see what happens).

drbyte commented 5 years ago

Ref: https://laravel-news.com/allowing-users-to-send-email-with-their-own-smtp-settings-in-laravel