Don't Prompt the User for Confirmation on the migrations:migrate Command When There is Nothing to Do
#480
Conversation
This brings them inline with some of the other, newer command tests that use Symfony's command tester. There's more coverage as well. Done so some changes can be made to `MigrateCommand`.
Removes a level of nesting from the rest of the method so some changes can be a bit nicer later on.
Provides the same output to the user as before.
Provides a way to hook into the migration and ask the user for confirmation. Right now that happens further up stream in a console command.
Means there's no longer a big warning or confirmation when no migrations are to be executed.
|
@chrisguitarguy Thanks It will take a little more time to review that PR for me but don't hesitate to ping me if I don't do it by next week. |
| @@ -118,12 +118,11 @@ public function setNoMigrationException($noMigrationException = false) | |||
| * @param string $to The version to migrate to. | |||
| * @param boolean $dryRun Whether or not to make this a dry run and not execute anything. | |||
| * @param boolean $timeAllQueries Measuring or not the execution time of each SQL query. | |||
| * | |||
| * @return array $sql The array of migration sql statements | |||
| * | |||
There was a problem hiding this comment.
please keep these lines separating params from the return value. It makes things more readable IMO
|
@chrisguitarguy @mikeSimonson What's the status on this pull request? This would be an extremely helpful improvement. |
|
I will review and merge this tomorrow without fault. |
|
Let me know if you need anything @mikeSimonson, I kind of lost track of this one. |
|
Any updates here @mikeSimonson? It's okay to reject too, I know it's a big-ish change. |
|
@chrisguitarguy As I said my only reserve is the change of the signature of the migrate method to make it return false. What do you think about that ? Wouldn't it be better to make it return an empty array ? |
I guess I was looking for a way to signify the difference between no migrations being found and the cancel callback failing. This means that the command can exit with a
Thoughts? |
|
Wouldn't you consider returning 0 when the user cancel ? After all there was no error. |
That's fair. I'll make the change tonight! |
Because it's not really an error. This also reworks the console command to keep the same interface: returns a 1 status code on cancel and outputs "migrations cancelled"
|
@mikeSimonson got back to this a bit late, sorry. I remember why I did the I reworked |
| * | ||
| * @return array $sql The array of migration sql statements | ||
| * @return array|false An array of migration sql statements or false if the confirm callback denied execution |
There was a problem hiding this comment.
The only thing that bothers me in this PR is the return array or false here. Wouldn't it be better if it returned an empty array in this case ? @chrisguitarguy What do you think.
I can make the change tomorrow if you agree to it ?
|
@chrisguitarguy Thanks |
Possibly closes #306
Couple big things here:
MigrateCommandso I could feel more comfortable changing itMigration::migrate.Migration::migrateto take an optional fourth argument for "confirm" the migrations can execute. This is similar to howOuputWriterabstracts away the Symfony console output inMigration. The default behavior is to always "confirm" the migrations if no$confirmcallback is given (same as it is currently).Not sure if adding another argument is okay, but it seemed to make the most sense. Even if none of the changes are okay, I do think the tests for the
MigrateCommandare useful. Let me know!