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

Allow for multiple Tenant ID's to be included #53

Closed
wants to merge 1 commit into from

Conversation

inctor
Copy link

@inctor inctor commented Dec 27, 2016

In some cases, we've seen the requirement for allowing multiple tenants to be included on the same platform/instance.

This modification should not have any implications on existing implementations, and should only extend the current functionality.

Since it's already allowed to use addTenants with an array, it makes sense, that it should also scope by multiple ID's.

Could be extended to allow for a collection object, instead of only arrays if it has potential.

In some cases, we've seen the requirement for allowing multiple tenants to be included on the same platform/instance.

This modification should not have any implications on existing implementations, and should only extend the current functionality.

Since it's already allowed to use addTenants with an array, it makes sense, that it should also scope by multiple ID's.

Could be extended to allow for a collection object, instead of only arrays if it has potential.
@inctor
Copy link
Author

inctor commented Dec 27, 2016

Granted the "is_array" statement may be slower in older versions of PHP, could be switched out in favor of
(array) $id === $id

But since PHP7, the performance difference between them is really small, even on large test-sets.

@0xn3bs
Copy link

0xn3bs commented Jan 30, 2017

@inctor this looks similar to my pull-request #17 which was rejected. Your implementation is a lot less invasive than mine, however it looks like there are no plans to support multiple tenants.

@inctor
Copy link
Author

inctor commented Jan 31, 2017

@inkadnb Yeah seems like it.
We've however stopped using the composer version of this package, and made the modifications to our own version, so we ensure that we have the needed functionality.

I know the functionality might be beyond the "scope" of this package, however, we have some specific needs for unique clients, that needs this functionality.

Eventually we'll write our own scope-package to handle this, but for now, we're just using a modified version of this package to accommodate our current needs.

@0xn3bs
Copy link

0xn3bs commented Jan 31, 2017

@inctor that's what we've ended up doing as well. Basically, we've made our own implementations of the most recent two pull-requests. Our main use-case is for administration, being able to scope multiple tenants at once and also being able to support tenant hierarchies. We also make tenant_id nullable so that certain data can span across all tenants.

@hipsterjazzbo
Copy link
Owner

Yeah, unfortunately, this is out of scope for this package. This package is meant specifically for scoping to a single tenant at once.

pjeutr added a commit to pjeutr/Landlord that referenced this pull request Mar 19, 2019
Fix for multiple tenants - hipsterjazzbo#86
Allow for multiple Tenant ID's to be included - hipsterjazzbo#53
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.

3 participants