MikeHopley's avatar

ConfirmableTrait is sub-standard

A while ago Taylor added ConfirmableTrait to destructive Artisan commands (migrations and seeding).

The idea is good. We want to protect the production database from being wrecked when a "dev oriented" command like migrate:refresh is accidentally run in the production environment.

But the implementation is poor. All you get is a "are you sure?" prompt, where pressing enter a second time is equivalent to "yes".

But that's not the real problem with it. The real problem is that it's a trait, not a service. This makes it difficult to replace with another implementation (my idea was artisan:lock and artisan:unlock commands).

Every other time I've wanted to change something about Laravel, it's been easy to extend or replace individual services. This means I can avoid directly editing the Laravel core, and make only the least changes necessary. When Laravel updates, I can expect everything to continue working.

To change ConfirmableTrait, I either have to edit the core ConfirmableTrait class itself, or I have to edit the core artisan command classes (like MigrateCommand) that use it. I can't just swap out the implementations, because it's not a service provider and it's not resolved out of the IoC container.

When something like this looks sloppy, I guess it shows how much I've come to expect from Laravel!

Or have I missed something? Is there a simple way to replace the behaviour of ConfirmableTrait without editing core Laravel classes?

0 likes
12 replies
MikeHopley's avatar

I found a way to do this without editing any core files. The end result is that both my custom "confirmation" and the default confirmation will be run -- but that's okay.

The basic idea is to replace the MigrationServiceProvider and the SeedServiceProvider with your own classes, which extend the original ones. Then override all the methods that register a command, and feed them your own command "wrapper" class instead (which extends the core command). Then you can add the extra check in your new "wrapper" command.

If anyone is interested, please say so, and I will write out full instructions.

rawfan's avatar

Hey Mike. Thanks for sharing. The question is: what would be the best way to fix the original problem. Maybe it should be fixed upstream instead of being worked around. I.e. we could fix your problems with ConfirmableTrait (less intrusive) or actually make it a Service (more work).

MikeHopley's avatar

Thanks @rawfan. I'll try to explain how I see the issue:

The reason I wanted to change its behaviour is that I feel it doesn't do its job well enough.

As a business owner who runs his own website, I feel very, very nervous about destructive Artisan commands in production. ConfirmableTrait is much better than nothing, but it is prone to failure.

It's all too easy to accidentally press "enter" twice and "confirm" the command. That's because "enter" is the last key you pressed. It's also easy to dismiss prompts without really reading them sometimes. Maybe you were in a rush; maybe you were expecting a different prompt and just "okayed" on through.

I wanted something safer, that would not be too much hassle either. The system I designed works like this:

In production, migrations/seeding are disabled; attempting to run them produces an error, not a prompt. You must unlock Artisan first: php artisan db:unlock. There is no way anyone would accidentally do this -- it's matching the command to the user's intent.

After Artisan is unlocked, these commands can be run. You can then re-lock Artisan using php artisan db:lock. You can also configure a cron job to run this command if Artisan has been unlocked for more than (say) 5 minutes.

I have it configured so that the default state is unlocked, and then the cron job (a Dispatcher scheduled command) is applied to whatever environments I want. This is more flexible than hard-coding the feature for production only; you might have other environments that you want protected.

However, as an alternative solution, I think ConfirmableTrait would be okay if "enter" did not confirm it.

rawfan's avatar

Are you sure ENTER will confirm in production? Because that would actually be a bug. The default is set to "false" (in 4.2 and 5). See ConfirmableTrait and Command which overide Symfony ConfirmationQuestion's default of true.

MikeHopley's avatar

Yes, I tried it. Not in a "real" production site, thankfully, but a copy. I will check again soon, just in case I made a mistake.

I actually quite like the use of a trait here; it seems elegant. The only difficulty is not being able to change it. So how about storing the class name in configuration? Then it can still be a simple trait, but it can be replaced with something else from within the App.

MikeHopley's avatar

I can confirm that "enter" confirms the command.

If this is a bug, should I open an issue on github?

rawfan's avatar

I cannot confirm this behavior. I just needed to run a migration in production and it did in fact cancel the migration on enter. I also tried a fresh install of Laravel 5, same.

➜  l5  php artisan migrate
**************************************
*     Application In Production!     *
**************************************

Do you really wish to run this command? [y/N] 
Command Cancelled!
rawfan's avatar

@isimmons this appears to be it. I just tested with a fresh install of Laravel 4.2 and the default behavior was also to abort, as the sources lead me to believe in the first place.

➜  l42  php artisan migrate
**************************************
*     Application In Production!     *
**************************************

Do you really wish to run this command? 
Command Cancelled!

This is actually the commit that changed the behavior.

2 likes
MikeHopley's avatar

I'm on 4.2.1. That explains it. Thanks!

Now that the default is false, I think the ConfirmableTrait is just fine. I might even undo my customisation (I'm not sure yet).

rawfan's avatar

@Mike Hopley I moved a medium-sized app to Laravel 5 in a day and I'm pretty sure more experienced people can do it in less time.. just FYI. ;-)

MikeHopley's avatar

I might give it a day and see. But at some point you have to say, "I'm not adding more features to this project". I'm at that point.

Laravel 5 is still quite new. I'm concerned about package support, particularly the Codeception integration. It might be a lot less effort to just wait for stuff to get fixed.

Please or to participate in this conversation.