laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

migrate reset commands shouldn't be available in production #1358

Open zeyad82 opened 6 years ago

zeyad82 commented 6 years ago

I don't know if this happened with anyone before. I have mixed between two instances of a project and ran php artisan migrate:fresh --force --seed on an old production instance instead of the new one which is also in the same machine. It would be great if FreshCommand, RefreshCommand and ResetCommand not to be available in a production env without having an explicit flag in .env set to true.

fletch3555 commented 6 years ago

Sounds like a perfectly reasonable pull request opportunity.

sisve commented 6 years ago

There's already such a flag in .env, and having it (APP_ENV=production) means you get a large warning about this when you run the command. But you've chosen to override that warning with the --force flag. You basically told the command that you knew what you were doing.

I think this is a workflow problem, you're used to use --force instead of using the warnings and checks already in place.

I don't see how this new check would be implemented. Everyone running php artisan migrate as a part of a deploy script would need it (or a --mega-force flag that overrides it), and then some sed-scripts to remove the flag (or just leave it in). It just adds complexity to something I think already works well enough.

However, if this were to be implemented; we would need to make sure the commands still exists and output helpful information on how to enable them.

fletch3555 commented 6 years ago

Very good points @sisve. I failed to notice the use of --force in the OP

zeyad82 commented 6 years ago

I used --force as it was supposed to be a new production instance, I don't think there is a need to have a migrate:fresh command for a production database without a flag in .env, why would we need that anyways?no one would reset production database everyday, otherwise he would set the flag to true in config or .env

sisve commented 6 years ago

I agree that there are many commands that are rarely executed in production. The app:name and make:* commands would also be weird to do in a production environment. Should these also be hidden when in production? If not, should these have separate flags for consistency?

zeyad82 commented 6 years ago

Exactly! A separate flag for those commands in config is reasonable to avoid running them mistakenly in a production env.

tomschlick commented 6 years ago

We could also define an array of allowed environments / blacklisted environments it can run in instead of a boolean "can run in production" flag.

public function allowedEnvironments()
{
    return ['local', 'testing'];
}
deleugpn commented 6 years ago

migrate:fresh was very useful for me when I was building a new database scheme for my company. Given a lot of mistakes I've made during the development I had to trash and rebuild the new 'production' database a couple of times. I agree with sisve first statement. There's already a protection system in place and if people choose to bypass it, they're going to get in trouble. Adding blacklist / whiltelist flags just move the problem elsewhere until someone comes and says that they destroyed production database by whitelisting it.

zeyad82 commented 6 years ago

Whitelisting won't be just a flag like force, it will be in database config for example, who you will do it only on purpose if you need it and remove it after that..