fix added support for --force option when migrating the database

[IsaiahPaget]

Fixes: #1332

Added the --force option to the winter:up command, in order to align with how Laravel handles this in their migrate command. I also added the ConfirmableTrait and uses a method from it to print a confirmation message to console if APP_ENV=production.

This will be a minor breaking change for any users who are using winter:up or any of its aliases without the --force option when APP_ENV is set to production.

@LukeTowers To be 100% honest, I forgot to estimate this one, but it took about 1 story point, if we are going by 1 story point being about 4 hours, when you include discovery, writing, and testing.

[damsfx]

This will be a minor breaking change for any users who are using winter:up or any of its aliases without the --force option.

I don't really agree.
This change can have a major impact on CI/CD pipelines.
I think it should be released in a major version of WinterCMS.

[IsaiahPaget]

This will be a minor breaking change for any users who are using winter:up or any of its aliases without the --force option.

I don't really agree. This change can have a major impact on CI/CD pipelines. I think it should be released in a major version of WinterCMS.

Yeah, I get what you mean for sure, it's an easy thing to miss if you are not diligently reading the release notes.

[LukeTowers]

@damsfx what sort of impact will this have on your workflow? I was thinking that if anyone misses it when reviewing the next version they'll catch it on the first deployment and then it'll be one quick tweak to make sure they pass the --force flag when the application is in production.

[damsfx]

@LukeTowers The impact on my workflow would be minimal, I'd just have to adapt my CI/CD pipelines and Makefile files.
As long as it's something documented, I think it would be the same for many users.

[LukeTowers]

@IsaiahPaget could you submit a PR for the docs as well please?

[IsaiahPaget]

@IsaiahPaget could you submit a PR for the docs as well please?

Of course

[bennothommo]

I disagree with this change. Migrations are part of a normal process of keeping a site up to date and should be non-destructive, and I anticipate it will cause a lot of grief for people with CI workflows as @damsfx said.

[LukeTowers]

@bennothommo this has been the default behaviour in Laravel since 2014.

@IsaiahPaget to minimize disruption to existing installs, let's target wip/1.3 and let's use the config key database.migrations.confirm_in_prod. We'll add that with a default value of true to the default database.php in v1.3, but in the command itself the fallback value will be false for existing installs (so config('database.migrations.confirm_in_prod', false)).

[bennothommo]

@LukeTowers if I had any say in Laravel, I would've disagreed with it there too. 😛

Going forwards with migrations, by their very nature, should be non-destructive - the migrations form part of an upgrade to a plugin or library and not running them is the more "destructive" action so to speak, so changing the workflow of doing them, even as innocuous as this, is introducing inconvenience.

If it's going to be done as part of a major update, and documented, I have less of a problem with it. But I still don't see any benefit in introducing it. Have a prompt when going backwards or clearing the DB and re-migrating, sure, but not here.

[LukeTowers]

@bennothommo fair enough. The primary reason I wanted to add support was so that the CLI was more consistent between Winter and Laravel. I could be convinced to make the feature disabled by default even for new installs, but I still think we need to have the functionality.

[IsaiahPaget]

@LukeTowers @bennothommo
When you guys say disabled by default, does this mean maybe we would add an optional environment variable? What did you guys have in mind?

[bennothommo]

@IsaiahPaget your PR is targeting the next major release, so don't worry about changing anything in your PR for now. If this is going to be implemented, then at least it's in the next major release where it will be expected that people will read an upgrade guide or release notes where we will make this change clear.

[LukeTowers]

Suggested change

	if (!$this->confirmToProceed()) {
	if (config('database.migrations.confirm_in_prod', false) && !$this->confirmToProceed()) {

[IsaiahPaget]

@LukeTowers Sorry I had a dyslexic moment reading your comment, I read 'comment' as 'commit' and I have never seen this 'suggested diff' feature so I am just now realizing this is just a suggestion, not actually added to the branch.

But now I am following lol. In the database.php did you have something like this in-mind:

    /*
    |--------------------------------------------------------------------------
    | Migration Repository Table
    |--------------------------------------------------------------------------
    |
    | This table keeps track of all the migrations that have already run for
    | your application. Using this information, we can determine which of
    | the migrations on disk haven't actually been run in the database.
    |
    */

    'migrations' = [
        'confirm_in_prod' => env('MIGRATION_CONFIRM_IN_PROD', false),
    ],

[LukeTowers]

Pretty much exactly that @IsaiahPaget

[LukeTowers]

@IsaiahPaget See my latest comment on the PR. Laravel 12 made the database.migrations config item an array so it's easier for us to add our own keys in there rather than make a new config item entirely. It's also usually better to avoid interacting directly with env() in your code and instead rely on the config system where it can then use the env() helper if desired there instead.

Let me know if that makes sense or if the plan is still a bit unclear.

[IsaiahPaget]

@IsaiahPaget See my latest comment on the PR. Laravel 12 made the database.migrations config item an array so it's easier for us to add our own keys in there rather than make a new config item entirely. It's also usually better to avoid interacting directly with env() in your code and instead rely on the config system where it can then use the env() helper if desired there instead.

Let me know if that makes sense or if the plan is still a bit unclear.

Right I am pretty sure I am tracking, so as far this goes, nothing on my end is needed for now?

[LukeTowers]

Correct. This PR will eventually need the config value added to config/database.php as well as my suggestion applied, but first that file needs to be updated on the 1.3 branch (along with all the other Laravel configs) to align with the default Laravel 12 config files.

[IsaiahPaget]

@LukeTowers I didn't update the migrations => migrations to be an array, because the code in Winter/Storm uses it and apparently didn't like it when it was an array. I figured it could make sense to have some console config, maybe this would be better in a console.php. Let me know? Or we could make changes to Storm.