Allow table inspection in several database connections at once #1285
Conversation
fixes barryvdh#1141 Currently the package cannot handle schema discovery in more than a single database at once. In multi-tenant applications it is very frequent that there are at least two databases active at some point in time: landlord (data common to all tenants) and tenant (specific to one tenant). When the configuration variable `db_mapping` is not specified, it defaults to null which then makes the code strictly behave as before. Hence this introduces no breaking change.
src/Console/ModelsCommand.php
Outdated
| $database = null; | ||
|
|
||
| $connectionName = $model->getConnection()->getName(); | ||
| $database = config("ide-helper.db_mapping.$connectionName"); |
There was a problem hiding this comment.
I have not checked the code yet, but it's wrong to use the global helper here; see all the other instances in this file how they access the config (resolve via container):
$this->laravel['config']->get();This is important as not everyone runs this library on a full framework installation, some might only use the illuminate/ components and not have this defined.
There was a problem hiding this comment.
Correct, sorry about that. This is now fixed.
There was a problem hiding this comment.
Although the change looks small, TBH I'm not sure it should be merged.
There's no "official" way how to achieve this, multiple 3rd party packages have different approaches and different best practices.
Is there a chance we could make this available for hooking into for 3rd party libs to ide-helper?
Besides that, is there a way to even test is? I'd argue the majority is not using such an approach and we might break it in the future as we've no coverage for it
|
Giving a way to hook into this part of the code would be fine for us. The issue we have is that there is no way to plug any logic into that part of the code as of now. That said, this is not even a matter of multi-tenancy: any app which uses 2 or more DB connections to fetch it's data from is exposed to the same issue. Multi-tenancy is just the most common use case for such a need. To me, allowing to specify a physical DB name in front of a logical connection name is not specific to multi-tenancy. Again, if I store data in 2 different DBs, I'd see the same issue. |
fixes #1141 and probably fixes #1280 too which looks like a side effect of that original issue.
Currently the package cannot handle schema discovery in more than a single database at once. In multi-tenant applications it is very frequent that there are at least two databases active at some point in time: landlord (data common to all tenants) and tenant (specific to one tenant).
When the configuration variable
db_mappingis not specified, it defaults to null which then makes the code strictly behave as before. Hence this introduces no breaking change.Summary
Type of change
Checklist
composer fix-style