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

Contract IsTenant replace Tenant uses #544

Merged
merged 20 commits into from
Jul 24, 2024
Merged

Conversation

masterix21
Copy link
Collaborator

@masterix21 masterix21 commented Jul 18, 2024

PR follows the discussion here: #543

@masterix21
Copy link
Collaborator Author

@freekmurze, I'm ready for your verification. I'm in love with these changes.

@freekmurze
Copy link
Member

Looks good! Maybe also drop support for Laravel 10 and provide upgrade instructions in UPGRADE.md 👍

Thanks for your work on this.

@masterix21
Copy link
Collaborator Author

I'm ready. Can I merge?

@freekmurze
Copy link
Member

Do you have other breaking changes in mind? Now would be the time to implement those too.

@masterix21
Copy link
Collaborator Author

I think not. I changed a few things in the tests, so apart from what I mentioned in the upgrade guide, it currently seems that there are no other breaking changes.

@masterix21
Copy link
Collaborator Author

A possible idea could be to use the Context instead of the service container with the currentTenant.

https://laravel.com/docs/11.x/context#main-content

@benddailey
Copy link

I think using the new context feature could be a really great idea especially since you are dropping Laravel 10 support and releasing a new major version. I remember coming upon this tweet shortly after context was released. Maybe it can provide inspiration? https://twitter.com/davidhemphill/status/1770893521748123683 Thanks @davidhemphill

@masterix21
Copy link
Collaborator Author

I have made some test using context, but when you make a tenant as current and then call Context::add('tenantId', 1), every job will contain the context if it is NotTenantAware.

We can reduce our code, but we should remove our TenantAware and NotTenantAware from the jobs. The only advantage is that it will help with all jobs/events dispatched by other packages.

@freekmurze: What is your opinion about that?

@freekmurze
Copy link
Member

Using context seems like the way to go. Let's make the context key, tenantId, configurable.

every job will contain the context if it is NotTenantAware.

That's no problem I think

We can reduce our code, but we should remove our TenantAware and NotTenantAware from the jobs. The only advantage is that it will help with all jobs/events dispatched by other packages.

I don't understand why this should be removed, could you give some more details on that?

@freekmurze
Copy link
Member

Ready again for review?

@masterix21
Copy link
Collaborator Author

Yes

@freekmurze
Copy link
Member

It all looks very good, thanks for your work on this!

@freekmurze freekmurze merged commit 8a22870 into main Jul 24, 2024
5 checks passed
@freekmurze freekmurze deleted the refactoring-interface branch July 24, 2024 11:34
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.

4 participants