Add extension rollback command

[clarkwinkelmann]

I'm not sure this adds any end-user benefit but this would really help extension developers when they're trying to run your migrations over and over again.

This also fixes migrateDown not returning the list of notes.

This PR adds a command that is used like this:

$ php flarum migrate:rollback flarum-auth-twitter
Rolling back extension: flarum-auth-twitter
Rolled back: 2015_09_15_000000_add_twitter_id_to_users_table
DONE.

Given that command will probably only be used by developer or when troubleshooting, I'm not sure it's worth checking if the extension is enabled or not. Or maybe rollback could be forbidden for enabled extensions and we could add a --force flag to do it even if the extension is enabled ?

Maybe you'd prefer having the extension name parameter as an option instead of an argument to keep backwards compatibility if you add more options (core or migration-file-specific) to migrate:rollback in the future ?

[clarkwinkelmann]

Just curious, what's the name or the standard you use for the spacing ? phpStorm always adds spaces between 'text' . 'text' and removes space between !$var and I always fall in the StyleCI trap 😬

[tobyzerner]

Does this roll back all migrations for a specific extension? That's confusing behaviour given that the Laravel migrate:rollback command will only roll back the last batch of migrations that were run.

Perhaps this command should be renamed to migrate:reset, which is the equivalent command name for this behaviour in Laravel.

In an ideal world, we should really be extending Laravel's migration stuff rather than rolling our own like we are now. I wonder if it'd be possible to use some of the Laravel classes as a base and override methods as necessary...

Don't think we need a --force flag at the moment.

I think inline with the generate:migration command that extension should be an option rather than an argument.

StyleCI rules are here: https://github.com/flarum/core/blob/master/.styleci.yml

[clarkwinkelmann]

Yes it resets all migrations for an extension, given there's no step system in Flarum. Yes using migrate:rollback/reset --extension= is the other option I was thinking about. Should I rename to reset and use an option instead of argument then? Le ven. 9 févr. 2018 à 23:41, Toby Zerner <[email protected]> a écrit :

…

Does this roll back *all* migrations for a specific extension? That's confusing behaviour given that the Laravel migrate:rollback command <https://laravel.com/docs/5.6/migrations#rolling-back-migrations> will only roll back the last batch of migrations that were run. Perhaps this command should be renamed to migrate:reset, which is the equivalent command name for this behaviour in Laravel. In an ideal world, we should really be extending Laravel's migration stuff rather than rolling our own like we are now. I wonder if it'd be possible to use some of the Laravel classes as a base and override methods as necessary... Don't think we need a --force flag at the moment. I think inline with the generate:migration command that extension should be an option rather than an argument. StyleCI rules are here: https://github.com/flarum/core/blob/master/.styleci.yml — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1363 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBTrHi0ABCmXFs_juymYs0NS1d8lHToks5tTMmMgaJpZM4SARsH> .

[tobyzerner]

Sounds good. I've made an issue #1365 to track future refactoring.

[franzliedke]

How about wrapping tasks like these in a php flarum extension foo/bar command with subtasks?

[tobyzerner]

I think I prefer an option, because it keeps things simple (eg. when you do php flarum help, you get a flat list of all of the available commands and don't have to worry about sub-commands or anything)

[franzliedke]

That was my point as well, when running php flarum extension foo/bar, you would obviously get a list of (currently available) subtasks. For active extensions, this list would be different from that for inactive ones.

[tobyzerner]

Ah I see, that could be quite handy actually. 👍

[clarkwinkelmann]

I made the changes.

I also added the message "No extension specified. Please check command syntax." if you don't specify the --migration option, because it seems you can't make options being required (which seems kind of logic)

[tobyzerner]

If an extension isn't specified, should we reset core migrations?

[clarkwinkelmann]

I didn't implement it because it doesn't seem to make sense. Unless steps are implemented this will delete the whole forum. Well maybe for development it can be useful.

More a recipe for disaster if somebody types it by mistake on their forum I guess 😬

Or maybe add another flag, like --core to run these ? Just like in the Migrate command, there will be quite a few lines to copy-paste to run the reset.

[clarkwinkelmann]

Also, if somebody runs the core down migrations without first running every migration down migrations, the migrator will be left in a broken state. If any extension changed the columns of a core table, you won't be able to run these down if the column (and table) was removed when reseting core. Even if you re-run migrate, these extension migrations will still be marked as run but the database won't reflect them.

The command could check whether all extensions were reset before resetting the core maybe ?

[tobyzerner]

Sound reasoning. I think we can leave it out for now.

[franzliedke]

@clarkwinkelmann Am I missing something, or would running down migrations for core and active extensions work properly when not providing an extension?

[tobyzerner]

I suppose it would. But it would be dangerous, we'd need a warning/confirmation.

[clarkwinkelmann]

@franzliedke it would be easy to implement the opposite of the migrate command, however this is not only dangerous but this might simply not work at all.

If you only rollback active extensions, but inactive extension have been previously enabled, you're stuck in a state where you need to rollback every single migration that has ever been run before you can safely rollback the core extensions.

Resetting core would probably require more checks to be able to do it safely, for both database integrity and prevent user mistakes.

Maybe instead of allowing to reset core we could just add a migrate:fresh behavior like in Laravel ? Don't try to run down anything, just delete the lot and start again. That's probably what you're going for anyway, given there are no migration steps.