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

Feature run migrations based on config #64

Closed

Conversation

mefenlon
Copy link
Contributor

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.

//Database/Migrations/2022_01_22_034939_create_languages_table.php
        if(config('world.modules.languages')){
            Schema::create(config('world.migrations.languages.table_name'), function (Blueprint $table) {
                $table->id();
                $table->char('code', 2);
                $table->string('name');
                $table->string('name_native');
                $table->char('dir', 3);
            });
        }

@nnjeim
Copy link
Owner

nnjeim commented Oct 26, 2023

@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?

@mefenlon
Copy link
Contributor Author

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.

  1. 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.

  1. 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');
		}

@mefenlon
Copy link
Contributor Author

This also got me thinking about the refresh command. I added a check to make sure the modules are enabled before dropping the table.
2e39d40

@mefenlon
Copy link
Contributor Author

@nnjeim did you have a chance to review my comments in response to your concerns?

@nnjeim nnjeim closed this Jan 13, 2024
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.

None yet

2 participants