Closed mefenlon closed 7 months ago
@mefenlon Thank you for this input however please take into count the following: 1- countries, states and cities cannot be conditionally migrated as they consist the basis of the package. 2- timezones, languages and currencies can be optionally migrated however the condition should extend to control the deployment of the related routes too.
what is your input on the above?
First, I love this package. You have saved me a lot of effort. In my use case, I added your package to a project that already had a languages table. I could have changed the table name in the config to avoid the conflict. I then could have made a new migration to drop the uneeded tables. But, I was expecting the behavior to be different considering I could disable the modules in the config file.
In looking over the code, it looks like every module but countries can be disabled. There are other instances in the code that check for city and state modules.
//Actions/SeedAction.php line 189
//state cities
if ($this->isModuleEnabled('cities')) {
$stateNames = array_column($bulk_states, 'name');
$stateCities = Arr::where(
$this->modules['cities']['data'],
fn ($city) => $city['country_id'] === $countryArray['id'] && in_array($city['state_name'], $stateNames, true)
);
$this->seedCities($country, $bulk_states, $stateCities);
}
I am sure there is a use case that would only want countries. I do see that you could not disable states and enable cities. But, I understand if you don't want to disable the states and cities.
It looks like the routes already check if modules are enabled.
//src/Routes/index.php line 19
if (config('world.modules.states', true)) {
Route::get('/states', [Controllers\State\StateController::class, 'index'])->name('states.index');
}
if (config('world.modules.cities', true)) {
Route::get('/cities', [Controllers\City\CityController::class, 'index'])->name('cities.index');
}
if (config('world.modules.timezones', true)) {
Route::get('/timezones', [Controllers\Timezone\TimezoneController::class, 'index'])->name('timezones.index');
}
if (config('world.modules.currencies', true)) {
Route::get('/currencies', [Controllers\Currency\CurrencyController::class, 'index'])->name('currencies.index');
}
if (config('world.modules.languages', true)) {
Route::get('/languages', [Controllers\Language\LanguageController::class, 'index'])->name('languages.index');
}
This also got me thinking about the refresh command. I added a check to make sure the modules are enabled before dropping the table. https://github.com/nnjeim/world/commit/2e39d402aa71f26bfb82dba94d1b71c289cec8b9
@nnjeim did you have a chance to review my comments in response to your concerns?
These changes check the config for enabled modules before running the migration.
When using this project, I noticed a table would be created despite a module being disabled. This may not be the most elegant solution, but it prevents the migration from running when the module is disabled.