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

Route helper in job doesn't use tenant context domain #1218

Closed
giorgiopogliani opened this issue May 2, 2024 · 5 comments
Closed

Route helper in job doesn't use tenant context domain #1218

giorgiopogliani opened this issue May 2, 2024 · 5 comments
Assignees

Comments

@giorgiopogliani
Copy link

Bug description

I am sending an email in a job and I am passing to the template an url using the route helper. The email is sent with the central domain instead of the tenant context domain.

Logging tenancy()->tenant shows that the tenant is correct so I guess the tenant is initialized?

My setup is using multi database tenancy

  Drivers
  Broadcasting ................................... log  
  Cache ........................................ redis  
  Database ..................................... mysql  
  Logs ................................ stack / single  
  Mail .......................................... smtp  
  Queue ........................................ redis  
  Session ...................................... redis

My bootstrappers:

'bootstrappers' => [
        Stancl\Tenancy\Bootstrappers\DatabaseTenancyBootstrapper::class,
        Stancl\Tenancy\Bootstrappers\CacheTenancyBootstrapper::class,
        Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper::class,
        Stancl\Tenancy\Bootstrappers\QueueTenancyBootstrapper::class,
        Stancl\Tenancy\Bootstrappers\RedisTenancyBootstrapper::class,
]

Steps to reproduce

Dispatch a job from a tenant context and log any route using the route helper.

Log::info('NOT IN JOB:' . tenancy()->tenenat->id);
Log::info('NOT IN JOB:' . route('dashboard'));
dispatch(new MyJob());

The job class handle method

// ...
public function handle() 
{
    Log::info('IN JOB:' . tenancy()->tenenat->id);
    Log::info('IN JOB:' . route('dashboard'));
}

Logs in the laravel.log file:

[2024-05-02 12:37:43] local.INFO: NOT IN JOB: 1  
[2024-05-02 12:37:43] local.INFO: NOT IN JOB: http://tenant1.myapp.test/dashboard
[2024-05-02 12:37:45] local.INFO: IN JOB: 1  
[2024-05-02 12:37:45] local.INFO: IN JOB: http://myapp.test/dashboard 

Expected behavior

As tenancy()->tenant has the correct tennat I would expect the route helper to use the tenant domain.

Laravel version

10.48.7

stancl/tenancy version

v3.8.1

@giorgiopogliani giorgiopogliani added the bug Something isn't working label May 2, 2024
@stancl
Copy link
Member

stancl commented May 2, 2024

This isn't done by the package automatically because there's no way for the package to know what the correct domain is:

  • tenants can have multiple (sub)domains
  • there may be multiple central domains

You should manually update app.url or use app('url')->forceRoot() with the right hostname.

We've added a bootstrapper for this in v4 but it still requires configuration due to the reasons above.

@stancl stancl closed this as completed May 2, 2024
@stancl stancl removed the bug Something isn't working label May 2, 2024
@giorgiopogliani
Copy link
Author

giorgiopogliani commented May 2, 2024

Could we add a primary domain config somewhere, maybe as extra column in the domains table?

@stancl
Copy link
Member

stancl commented May 2, 2024

Yes, you can do that. It's up to you how you approach this, the right solution here will be application-specific.

@giorgiopogliani
Copy link
Author

giorgiopogliani commented May 2, 2024

Ok, might be a common use case, anyway I'll handle it. Thanks for the super fast reply!

@stancl
Copy link
Member

stancl commented May 2, 2024

In v4 we've added:

  • default configuration for the new bootstrapper that just uses domains->first() iirc
  • single domain tenants, for cases where you might not need a separate domains table. So the domain is just stored on a string column

In the SaaS boilerplate we use this primary domain concept. iirc it's a hasOne() relation from the tenant.

The package won't ship with primary domains out of the box since they only make sense when you really do need multiple domains and have some UI for letting the tenant manage their domains. But the changes we've made should add a good default implementation for this use case.

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

No branches or pull requests

2 participants