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

S7k/non tenant side #169

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sniper7kills
Copy link
Contributor

Add tutorial for a basic "non-tenant" site setup along with some common problems, and tips that need to be accounted for.

Copy link
Member

@ArlonAntonius ArlonAntonius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should look quite a bit better after these changes.
Can someone from @tenancy/team review this as well? :)

// Get our tenant (A Customer)
$customer = App\Models\Customer::firstOrFail($request->get('tenant_id'));
// Tell tenancy, that we want to access the above tenant
Tenancy::setTenant($customer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend using the ConnectionResolver in order to set the connection, rather than completely setting the tenant as per performance, security etc.

// And we can access the information stored in their database.

// We will assume that Products uses the `OnTenant` Trait
$tenant_products = Products::all();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

``$tenantProducts` please :D

### Accessing `OnTenant` models from a "sub-site"
Before a model that uses the `OnTenant` trait can be used a tenant needs to be identified. Because of this it is best practice to avoid using tenant models on a non-tenant site.

In events where this access is still needed, a tenant will first need to be identified within your code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could need it, or not, please try to make it look more like an option. Maybe showcase both the ConnectionResolver way and the setting tenant way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even think this warning is necessary. Especially on the tenant website you will need to know who the tenant is.. In fact I would even enable eager tenant identification for this.


## Tips and Reminders
### Namespaceing
As with any Laravel application; if you are storing controllers in a different location than in `app\Http\Controllers` you will need to use the namespace option within your routes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line and the usage of namespace, please. I don't personally recommend using namespaces, rather classes to reference the controllers

```

### Routes Affect Listener
Next we need to create the listener for the [Routes Affect]() that will get rid of the system (non-tenant) routes and load our tenant route file instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to the right page please

@fletch3555
Copy link
Member

Agreed with @ArlonAntonius's comments. Once those are fixed, this should be good to go

Copy link
Contributor

@luceos luceos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great tutorial, loved reading it!

This tutorial will assume that you have followed the [basic setup tutorial](tutorial-basic-setup).

## Routes
The first thing we want to do is configure the [Routes Affect](affacts-routes).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The first thing we want to do is configure the [Routes Affect](affacts-routes).
The first thing we want to do is configure the [Routes Affect](affects-routes).

## Routes
The first thing we want to do is configure the [Routes Affect](affacts-routes).

First start by creating a new file for all tenant specific routes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is repeated from above. First we do, first we do that.

A more fluid way of saying this is something like

Suggested change
First start by creating a new file for all tenant specific routes.
In order to set up the tenant specific routes, we will need to create a routes file for it.

*/

Route::get('/', function () {
//This is the "root" route for all tenants
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd never use a callable in an example for routes. Callables cause routes files to be no longer cacheable. So.. either just remove the non-working example or provide a controlller from the get-go.

```php
protected $listen = [

...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
...
// ...

App\Listeners\ConfigureTenantRoutes::class,
]

...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
...
// ...

});

Route::get('/shared', function() {
//This is a route that is shared with both the admin site and the public site
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No callables.

})
```

### Accessing `OnTenant` models from a "sub-site"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename "sub-site" to tenant site or something else? And make it consistent in the whole tutorial.

### Accessing `OnTenant` models from a "sub-site"
Before a model that uses the `OnTenant` trait can be used a tenant needs to be identified. Because of this it is best practice to avoid using tenant models on a non-tenant site.

In events where this access is still needed, a tenant will first need to be identified within your code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even think this warning is necessary. Especially on the tenant website you will need to know who the tenant is.. In fact I would even enable eager tenant identification for this.

Comment on lines +156 to +157
When the User Model has the `OnTenant` trait applied to it, you will not be able to have those users login to any "non-tenant" routes. Because there is no tenant identified, it will not be possible to authenticate the user.
To overcome this you will need to either create two different User models (Admin & User), or will need to store all users in the system database. The option you choose will depend on youre goals and objectives.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true and reminds me a lot about the restrictions that hyn/multi-tenant had. If you would want an admin dashboard that allows authenticating users from a tenant, you can create a log in page that asks for their credentials and one more field with a slug, unique hash or something else to identify the tenant with. Then, modify the authentication layer to see whether the identifier is in the payload and set up a tenant database connection on the fly..

t/t is built to be as flexible as possible, the only limitation is your own imagination.

Comment on lines +160 to +161
When using subdomain identification, it is best practice to preform an additional check when registering a new tenant that they do not try to use a subdomain you have configured for a "non-tenant site".
I.E. Do not allow users to register the subdomains (`www`, `admin`, etc...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without providing an actual implementation for this, I'm not sure this advice is appreciated. Even mentioning this might sound condescending because it seems pretty obvious you want this.

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