Fix issue 515: Cannot create a new tenant with a different different DB host

[babisp]

[stancl]

Hey, thanks for the PR!

Could you explain what was the issue with the original behavior, and exactly what these changes do?

[babisp]

Hello!

In our app, we have a form that creates tenants dynamically. In that form, we can set tenancy_db_* variables in order to override the default ones of the tenant template in config/database.php.

Using that form, I set the tenancy_db_host variable because we have multiple database servers for our tenants.

This works properly with the database tenancy bootstrapper, however it does not work for the creation and the deletion of the tenant database. The database is created (or deleted) in the host that is originally set in the template in the database config file. Then, tenancy tries to run the migrations in the correct host, where the database does not exist, of course.

With the changes I have proposed, the template in the app's config file is temporarily merged with the tenant's config, so that the database can be created.

[stancl]

I think this PR applies changes on top of what's being done now, but this could be addressed at a lower level.

The $tenant->database() method uses setConnection():

https://github.com/stancl/tenancy/blob/27e9fb4a692a15ca2fd320799d97fc657cb14b68/src/DatabaseConfig.php#L149-L165

The solution is probably to make the created connection correct here.

Or am I missing something? I haven't worked with this part of the codebase for a bit so I may be wrong. Thanks!

[stancl]

Right, so we need to create a special type of connection, because it must use the tenant's DB server, but not the database name, since it's not created yet.

I went through your changes and I they could be simplified by using the setConnection() logic in conjunction with the method you added for creating the host connection — which is implemented very nicely.

I'd revert the DatabaseManager changes and make the create & delete jobs use code like this:

config(['database.connections.tenancy_database_manager' => $tenant->database()->hostConnection()]);

$manager = $this->tenant->database()->manager();

$manager->setConnection('tenancy_database_manager');
$manager->createDatabase($this->tenant);

What do you think?

[stancl]

And the config() call could be abstracted in DatabaseManager, yeah. But I'd use a custom name rather than storing the original name and dealing with conflicts and potential caching on the PDO side.

[babisp]

Hey, thanks for the review. I think I agree with your comments, after all it's the first time I dived into the package's code so I wasn't quite sure about where to place the changes.

So, please confirm you are suggesting the following:

• Revert the DatabaseManager to its original state (remove createHostConnection and resetTenantConnection)
• Adjust the two jobs to create a local DatabaseManager instance and set its connection before creating/deleting the database

Is that right?

I am not sure I understand your last comment.

Also, how about the DatabaseManager::ensureTenantCanBeCreated() method? Maybe it should be adjusted too?

[babisp]

I do get the custom connection name part but could you please explain the abstraction part a bit more?

[stancl]

By abstracted I mean that the config() call shouldn't be done inline like in my comment, but should use a method on DatabaseManager, like this

https://github.com/stancl/tenancy/blob/54a33f93a85a5e0a4d53ac48fabacd8ebb716eec/src/Database/DatabaseManager.php#L70

but named createManagerConnection() or createHostConnection(). Maybe manager connection is better, because it specifically sounds like creating a connection for the needs of the DB manager.

Nice catch with ensureTenantCanBeCreated(). At that point I think it might be better to create a single method like $tenant->database()->manager(), but for the host manager, so maybe hostManager(), and use this in all of the places?

Meaning, ensure DB can be created, and the create/delete jobs.

[stancl]

Okay, so I think you should do this. Ignore some of the suggestions above, this is a better solution IMO.

1. Add a method like this, name it hostManager().
2. In that method, use 'tenancy_db_manager' here, instead of the tenant connection
3. Before that call, create the host connection by calling config(['database.connections.tenancy_database_manager' => $this->hostConnection()]);
4. Update the Create DB, Delete DB, and ensureTenantCanBeCreated methods to use ->hostManager() instead.

[vahidalvandi]

have good idea

[bogdankharchenko]

Interested in this getting pushed, @babisp @stancl anything I can do to help move this?

[stancl]

If the original author isn't working for this, you can fork his code, create your own PR, and resolve the issues in the review.

[stancl]

Just a note: I changed some logic related to the DB template logic. So there might be slight differences in behavior now.

[stancl]

Closing this now, will re-open the issue and link it to #946