Rollbacks by start time

[ghost]

A new Configuration option named version_order was added. Its possible values are:

• creation-time (the default): causes the Rollback and Status commands to order the executed migrations by their creation times (aka Migration IDs or Names).
• start-time: causes the Rollback and Status commands to order the executed migrations by their start times (aka Execution Times).

This means that when invoking the Rollback command:

• With a start-time version order, the last executed migration will be rollbacked.
• With a start-time version order and a -d (date) option, the migration start times will be used to determine the target version to rollback to. In other words, all migrations which were executed after that date will be rollbacked.
• With a start-time version order and a -t (target version) option, all migrations which were executed after the target version was executed will be rollbacked.

In terms of testing:

• The old testRollbacksByDate test was renamed to testRollbackToDateTime and turned into a unit test, ensuring the rollback method is called with the right target version. A new testRollback was added which ensures the correct output of the rollback operation (as was being done in the old testRollbacksByDate test).
• More test cases were added.
• General test improvements were done (like adding the expected arguments to the underlying Manager calls in RollbackTest and StatusTest.

Other notes:

• The old rollback and rollbackToDateTime methods were merged into a single rollback method that handles all cases and version orders.
• This PR replaces Added option to allow for Rollbacks by start time #745.

[ghost]

@rquadling this is simply the rebase from 0.5.x-dev to 0.6.x-dev, where I had to adapt to the new breakpoint feature.

I still have to act on your #745 (comment)

[ghost]

regarding your comment:

A suggestion for phinx status, could a highlight be added to the column that is used for the ordering.

ok, but I'm not sure how to highlight the column

Maybe the rollback_order in the config could be version_order, allowing for the ordering to be used logically for phinx status also.

makes sense to me, shall I do it?

For the version ordering, if the key to the version log retrieval was the ID or the execution date (in CCYYMMDDHHIISS format), then the existing rollback methods COULD be used AND only a --target would be needed. No need to differentiate between the mechanisms for ordering.

hmm so you're saying like, get the versions properly ordered by creation or execution time from getVersionLog and then the rollback itself would be the same in either case? Makes sense, I dunno why I didn't do it like that initially :-) I'll have a look at it, as currently the methods to rollback by creation (which already existed) and execution time (which I added) are quite different.

Once the ordering is set, I've made my intent clear. --target should operate on that setting appropriately without the need for addition flags.

So no -s/--start_time command-line option, only the config option?

[rquadling]

That sounds about perfect. No -s/--start-time.

For highlighting, https://github.com/robmorgan/phinx/blob/0.6.x-dev/src/Phinx/Migration/Manager.php#L112 contains the column headings.

A simple variant could be to wrap the name with <info>[...]</info> or to use ===== as underline for the column.

Probably though the [...] would be best.

[ghost]

on it!

quick question: for the "version order" references, I believe it's much cleaner to have the valid values (strings start-time and create-time) defined somewhere as constant values so we can use them cleanly throughout the code, eg. doing:

// bad
if ('start-time' === $versionOrder)

Vs

// good
if (VERSION_ORDER_START_TIME === $versionOrder)

My question is, where shall I store these values? They are gonna be needed (I think) in the Rollback command, its test and the Manager classes.

[ghost]

Another question: for passing the version order from the Rollback command class (which gets it from the configuration file and checks its valid) to the Manager class (which obviously needs it), shall I pass it simply as a rollback method argument, or is there a better way? Just thinking that that method already has 2 optional arguments ($version and $force), if we keep adding optional parameters it might make it hard to call it with only some options (eg. ->rollback('env', null, null, $versionOrder).

Maybe I can simply read it from the Config class? The only other thing I'd need to do is set the key to its default value (creation-time) in the Rollback command class if it is not set.

[rquadling]

I would put the constants in Config, as they are configuration constants. With an appropriate getter(). Which will also include the validation and throw an exception if the value is incorrect/unknown. If not set then supply the default which would preserve the current status quo of creation date order.

So, then, in both status and rollback, something like $versionOrder = $this->getConfig()->getVersionOrder(); maybe.

[ghost]

perfect, looks good.

What about the rollback command's -d/--date parameter, does it still make sense to have it? From your comment:

Especially if the logic was merged, allowing something like --target 201604 . rollback to the beginning of April 2016. Either by version ID or by execution date, depending upon the setting of version_order (currently rollback_order within this branch).

It looks like we don't need a --date/-d argument at all? (also, I noticed it's not mentioned in the documentation).

[ghost]

there is a problem with:

For the version ordering, if the key to the version log retrieval was the ID or the execution date (in CCYYMMDDHHIISS format), then the existing rollback methods COULD be used AND only a --target would be needed. No need to differentiate between the mechanisms for ordering.

which is that whereas it is impossible to have two versions with the same create time (right?), it might happen that two versions are executed in bulk at the same time and end up with the same start time, meaning we can't really index the versions/migrations by start time without ensuring we won't ignore entries with the same start time (ie. array key)...

(sorry for all the questions, it's just nice to finally have someone in charge listening and advising hehe, so I might as well take advantage of that and ensure I do the best PR possible)

[ghost]

it is impossible to have two versions with the same create time (right?),

I double-checked since I didn't remember seeing any checks for this in the code, but the PRIMARY key in the phinxlog table ensures that version names (ie. their creation times) are indeed unique

[ghost]

we can't really index the versions/migrations by start time without ensuring we won't ignore entries with the same start time (ie. array key)...

I could index the migrations (returned in Manager.getMigrations()) and the versions (returned in PdoAdapter.getVersionLog()) by start time if the array values are arrays of versions instead of single versions.

This would allow for having several versions with the same start time, but it wouldn't be able to ensure any definitive version order for the same start time, which could lead to unexpected behaviour. Maybe I can sort the versions by start time and creation time (aka ID)?

[ghost]

just updated the initial PR description (copied from the old PR) with the new stuff from this PR

[rquadling]

If they are ordered by $versionOrderingColumn, 'version', then that should keep the order just fine.

With regard to the output for the status, try running your code with --ansi. Does this output anything visually? If not, I think we need an additional marker for the ordering.

[rquadling]

Is there no way to merge rollbackToDateTime() with rollbackToDateTimeByStartTime() and to merge rollback() with rollbackByStartTime().

It just grates having 4 methods that all do the same work. In essence. I may need to take a closer look at the workings of these.

[rquadling]

And I'm happy to be helping here. A lot of the features people are working on I will be actively using anyway.

[ghost]

If they are ordered by $versionOrderingColumn, 'version', then that should keep the order just fine.

on it.

With regard to the output for the status, try running your code with --ansi. Does this output anything visually? If not, I think we need an additional marker for the ordering.

the output is the same with --ansi or without anything, the "order-by" column is shown in green while the remaining columns stay white.

Is there no way to merge rollbackToDateTime() with rollbackToDateTimeByStartTime() and to merge rollback() with rollbackByStartTime().

It just grates having 4 methods that all do the same work. In essence. I may need to take a closer look at the workings of these.

I will try to merge them now - the only problem I had was the ordering, now that you replied I'm good to try it.

How about the -d/--date option? Stays or goes?

[rquadling]

--no-ansi (DOH!)

As for -d/--date yeah. I can now see they are both needed. But I'd expand date to work as:

2016 -> 20160101000000 / Midnight Jan 1st 2016
201603 -> 20160301000000 // Midnight March 1st 2016
20160503 -> 20160503000000 // Midnight 3rd May 2016
2016070810 -> 20160708100000 // 10:00:00 on 8th July 2016
201609121345 -> 20160912134500 // 13:45:00 on 12th September 2016
20160912134555 -> 20160912134555 // 13:45:55 on 12th September 2016

Basically, allowing me to use a point in time between 2 runs of the migration.

It would be nice to have a --dry-run option that would show the status page with the current up/down and the new up/down, taking into account any breakpoints. #906

[ghost]

--no-ansi (DOH!)

Indeed, with --no-ansi the status is displayed all in white and you can't tell the order-by column (sorry, only now did I understand where you were going with that). How would we signalize the order-by column in that case? Maybe an asterisk * next to the column name? Don't really know what the standard is...

As for the -d/--date option, I think I get the need for it.

Let me just summarize how I think the rollback mechanism would work, to make sure we are on the same page:

Premise: Depending on the configuration's version order, the existing versions (ie. the previously executed migrations) will be sorted by creation time (aka name) or start time (aka execution time).

Behaviour with a -t/--target option:

• Value 0: all migrations are rolled back.
• Version name of an existing (and previously executed) migration (eg. 20111018185412): all the migrations after that one (in the sorted list from the premise) will be rolled back.
• Version name of a non-existing migration: an exception is thrown.

Behaviour with a -d/--date option:

• All the migrations after that date (in the sorted list from the premise) will be rolled back.

Behaviour with no -t/--target or -d/--date option:

• The last migration (in the sorted list from the premise) will be rolled back.

Is that it?

(note that this description is ignoring breakpoints, the implementation of which is quite simple: if at any point a migration is about to be rolled back and it has a breakpoint set for it, the mechanism stops right there without rolling back that migration and prints the Breakpoint reached error message)

[rquadling]

Maybe changing the ------- under the label to ======= or add [....] around the label. See which looks good. Probably <info>[....]</info> would be my preference.

As for -d|-t ...

In both cases, date and target operate on the sorted results.

Depending upon the ordering, the status page would look like ...

By version

Status  [Migration ID]  Started
-------------------------------------------
    up  20160105000000  2016-01-05 00:00:00 - 1st deployment
    up  20160110000000  2016-01-12 00:00:00 - 3rd deployment
    up  20160115000000  2016-01-10 00:00:00 - 2nd deployment

or

By execution

Status  Migration ID    [Started          ]
-------------------------------------------
    up  20160105000000  2016-01-05 00:00:00 - 1st deployment
    up  20160115000000  2016-01-10 00:00:00 - 2nd deployment
    up  20160110000000  2016-01-12 00:00:00 - 3rd deployment

Now lets got through the some "targets" for rollback.

Target         Expectation
-------------- -----------------------------------------------
0              All migrations
2016           All migrations from 2016-01-01 00:00:00 onwards
201602         All migrations from 2016-02-01 00:00:00 onwards
20160304       All migrations from 2016-03-04 00:00:00 onwards
2016040506     All migrations from 2016-04-05 06:00:00 onwards
201605060708   All migrations from 2016-05-06 07:08:00 onwards
20160607080910 All migrations from 2016-06-07 08:09:10 onwards

For 'By version' supplying a target would rollback the following migrations.

Target   Rollbacks performed in the following order
-------- -------------
20160104 2nd, 3rd, 1st
20160105 2nd, 3rd
20160106 2nd, 3rd
20160107 2nd, 3rd
20160108 2nd, 3rd
20160109 2nd, 3rd
20160110 2nd
20160111 2nd
20160112 2nd
20160113 2nd
20160114 2nd
20160115 none
20160116 none

For 'By execution' supplying a target would rollback the following migrations.

Target   Rollbacks performed in the following order
-------- -------------
20160104 3rd, 2nd, 1st
20160105 3rd, 2nd
20160106 3rd, 2nd
20160107 3rd, 2nd
20160108 3rd, 2nd
20160109 3rd, 2nd
20160110 3rd
20160111 3rd
20160112 none
20160113 none
20160114 none
20160115 none
20160116 none

I see "target" and "date" being merged into a "time point", which has automatic expansion to a full datetime depending upon what parts are not present.

And that datetime is then used against the ordering. As the ordering is set at a config level, I don't see a need to differentiate once the migrations to rollback have been determined.

All migrations after the timepoint are rolled back, depending upon the sort order.

[ghost]

Maybe changing the ------- under the label to ======= or add [....] around the label. See which looks good. Probably [....] would be my preference.

easy peasy.

I see "target" and "date" being merged into a "time point", which has automatic expansion to a full datetime depending upon what parts are not present.

I'm with you, but doesn't that mean that -d/--date and -t/--target are the same then? If both of them are translated into a "time point" that is then matched against the executed migrations (regardless of how they were ordered as per the current configuration) to decide which ones get rolled back, then I can't see any difference between the 2 options. I thought that the difference was that the -v/--version was "stricter" in the sense that you had to pass an exact version name (regardless of the configuration version order), but reading what you wrote, it doesn't look like it... I mean, in your example you yourself simply wrote:

Now lets got through the some "targets" for rollback.

without distinguishing between a target version via -t/--target or a target date via -d/--date.

[rquadling]

It is just painful having -t 20160105121314 AND a -d "2016-01-05 12:13:14"

If someone needs a "strict" point, they will enter one. But it is also about enforcing a strict behaviour.

Fair enough.

So, because of the 'strict' nature of MUST having a matched ...

-d gets expanded to a full date time (the timepoint).
-t is already expanded and so is already a timepoint.

If the timepoint is from -d it doesn't have to exist as a date (version or execution, depending upon ordering).

If the timepoint is from -t it DOES have to match at least 1 datetime (version or execution, depending
upon ordering).

How does that sound.

And of course, that may be exactly what we do already! Ha ha! (ahem - I'm off home - sorry for wasting everyone's time!!!).

[ghost]

It is just painful having -t 20160105121314 AND a -d "2016-01-05 12:13:14"

That was precisely my point :-) I wasn't arguing for having the -d/--date, I was arguing against it haha.

As for the strict nature, I wasn't trying to suggest that that was how it should work, I was saying it's what I thought was wanted.

And of course, that may be exactly what we do already!

I think it kinda is :-) I just got confused when you mentioned that both target versions and target dates were a time point, pure and simple.

Anyway, me personally I think the -d/--date doesn't make sense: if we make the -t/--target simply be the "time point" you mentioned, I think we cover any possible needs. But that's my opinion, I'm happy to do it like that or keep both.

So the way I see it we have 2 options:

• Only have -t/--target: this value is converted to a date time and matched against the executed migrations (sorted as per the configuration version order) and any migrations that show up after it are rolled back.
• With both -t/--target and -d/--date: in this case, the executed migrations are still sorted as per the configuration version order, but now:
  • With a -t/--target, the value must match an existing migration's name (aka creation time) and all the migrations that are after in the sorted list are rolled back.
  • With a -d/--date, the value can be any date string and all the migrations that are after in the sorted list are rolled back.

What shall it be?

[rquadling]

I do feel that -d and -t are essentially the same. The one issue is the 'strictness' of -t. The requested target MUST exist.

So.

I think if we can support all the features with minimal code (i.e. not 4 methods that nearly all do the same thing, then I think that will be best. Keeping both -t and -d will be easier for end users as they already have that.

I think the code should do the following steps.

Get the migrations in the appropriate order. Keyed by version ID.
If -t, verify migration exists. If migration does exist, extract the appropriate timepoint (depending upon version ordering). If the migration does not exist, bail out.

If -d, expand to a full time point.

Rollback migrations in order until migration timepoint (depending upon the version ordering).

All handled by 1 method.

[ghost]

Get the migrations in the appropriate order. Keyed by version ID.
If -t, verify migration exists. If migration does exist, extract the appropriate timepoint (depending upon version ordering). If the migration does not exist, bail out.

If -d, expand to a full time point.

Rollback migrations in order until migration timepoint (depending upon the version ordering).

All handled by 1 method.

@rquadling done :-) let me know what you think

[ghost]

I see some failed tests in AppVeyor, although in Travis CI (and locally) everything passes. What specifically is AppVeyor testing, how dos it differ from Travis and how can I run that environment locally?

[rquadling]

AppVeyor runs Windows + MSSQL. I can re-run the tests locally and give some feedback.

I run Windows and can test MySQL, Microsoft SQL Server, SQLite and PostgreSQL. Possibly one of the good points about Windows is that it can run MS servers AND most other SQL engines. Yay for windows!

[rquadling]

You've changed the tests and have embedded unix line endings in the regex which will fail on Windows.

 -         $this->assertRegExp('/\s*up  20120103083300  2012-01-11 23:53:36  2012-01-11 23:53:37  *\*\* MISSING \*\*\n'.
 -             '\s*up  20120815145812  2012-01-16 18:35:40  2012-01-16 18:35:41  Example   *\*\* MISSING \*\*\n' . 
 -             '\s*down  20120111235330                                            TestMigration\n' .
 +         $this->assertRegExp('/\s*up  20120103083300  2012-01-11 23:53:36  2012-01-11 23:53:37  *\*\* MISSING \*\*'.PHP_EOL.
 +             '\s*up  20120815145812  2012-01-16 18:35:40  2012-01-16 18:35:41  Example   *\*\* MISSING \*\*' .PHP_EOL.
 +             '\s*down  20120111235330                                            TestMigration' .PHP_EOL.

The above is an example patch.

[ghost]

indeed I have! will get rid of that

[ghost]

💥

[ghost]

I'll add the new features to the docs now

[ghost]

To be honest, I was getting very confused with names like $migrations and $version, as in, which one refers to the migration classes and which one refers to the rows in phinxlog.

But maybe it's just me, so for the sake of consistency, I'll rename the variables.

In the meantime, I think I found a bug in the Status command, so hold off on the merge ;-)

[ghost]

bugs fixed, ready for more scrutiny!

[rquadling]

Unused?

[rquadling]

Rebase and squash as we are getting a lot of commits here. Everything is going in the right direction.

[rquadling]

Rebase is important as you are missing tests (and maybe code) related to breakpoints.

[rquadling]

Oh dear. I led you the wrong way. I forgot that the config class is, essentially, read only, no setters() needed. All guards are in the getters().

And looking at the rest of config, it doesn't really guard against much!

I think the getter can be as simple as:

return $this->values['version_order'] ?: self::VERSION_ORDER_CREATION_TIME;

[ghost]

so I should remove the setter and replace the getter with that? Where should the guard go to? Having an invalid configuration version order (because of a typo, for instance) might lead to unexpected behaviour

[rquadling]

Not necessarily. If you add the default: clauses, the the system will use that for an incorrect value. I think there are a limited number of places where the value is used.

[ghost]

so you mean, wherever where I do a getVersionOrder, like say in the Manager class, I do a switch clause with cases for creation and start time and default to raising an exception for an unknown order?

[ghost]

or it actually sounds like you're suggesting something like:

switch($this->getConfig()->getVersionOrder()) {
    case \Phinx\Config\Config::VERSION_ORDER_START_TIME:
        // handle start time version order
        break;
    case \Phinx\Config\Config::VERSION_ORDER_CREATION_TIME:
    default:
        // handle creation time version order
}

The problem I see here is that an invalid version order (eg. a typo like starttime) would be invisibly handled as a creation-time, which could lead to unexpected behaviour (since phinx would rollback the "invalid" commit, at least in the eyes of the user). I believe we should guard against this, if not on the Config getter/setter, then at least in the switch's default clause, by raising an exception. If we do so, the only thing left to do is to return the default version order (creation-time) in the getter if none was set in the configuration file.

[rquadling]

I would use default to be the default. So the current behaviour is in creation time order, so that's the default behaviour.

[ghost]

I would use default to be the default. So the current behaviour is in creation time order, so that's the default behaviour.

even if the user writes something else in the config file? that seems to go beyond the scope of "default"

[ghost]

rebased and squashed

[ghost]

@rquadling maybe could I make this message more nice and concise like your other comment:

Invalid date. Format is Y[m[d[H[i[s]]]]].

?

[rquadling]

The format is an acceptable way of describing a parameter. It is not uncommon to see nested optional parts. Unusual to see many as this, probably, not not unusual enough to need anything more complex.

I'd recommend Invalid value. Format is CCYY[MM[DD[HH[II[SS]]]]]. I'd also like to have the --help page show a few examples as well as mention it is not JUST date.

It is a bit confusing when -t is target (or timestamp) and -d is date (or datetime but as a timestamp).

All a bit samey.

[ghost]

all good, but, why CCYY and not YYYY? What does the C stand for? Century? Afaik, date formats simply refers to Y for years

[rquadling]

CCYY or YYYY, but never YY or Y.

[ghost]

so, I think currently we only have these open points:

• Should I rename the version_order values to creation and execution or keep them as creation-time and start-time (respectively)?
• The Config getter/setter where-should-the-guard-go-to discussion.

[rquadling]

I prefer the shorter form.

The guard for the current config options is based upon the consumer, not in the config class.

By having it in the config class, potentially we are restricting subclassing. Maybe.

For the guard, the default value is assigned during config and then using a switch, the default case could either be the same as the default value or an exception could be thrown.

Thinking ...

Exception. Basically, if someone has gone to the trouble of editing the config and done it wrong, BOOM!

If no config changes, then everything operates exactly as it is at the moment.

[ghost]

done! :-)

anything else missing??

[rquadling]

That's all looking very good!!! I'll be doing a thorough review tomorrow (and testing too. Hopefully unit tests have increased coverage.

One thing I've found in my own code, especially where there are many options all working together, generating the test data for unit testing can be as troublesome as the code itself.

So (doing this blind without looking at the tests), the following options all play a part in rollback.

1. Version order setting - null/undefined, 'creation', 'execution', invalid.
2. Migrations order - same order, reversed order, mixed.
3. Breakpoint settings - setting is after target, setting is on target, setting is after before target.
4. Rollback target - after all migrations, is an exact match for a migration, between 2 migrations, before all migrations.
5. Number of migrations - 0, 1, 2, 3+

So, just taking into account those options, potentially, 432 combinations. Some won't be valid (0 migrations for example), but the code still needs to respond appropriately.

Having a rollbackProvider that can present all these tests with their expected results or exceptions, and, ideally, a message indicative of the test data (Version order undefined, migrations in order, no breakpoint settings, target after all migrations with 0 migrations failed).

It is the large combinations of options that cover all bases that reveal those broken edge cases.

If need be, write a script (and commit it) that generates the tests. That way, if the fundamentals change, we can change the generator and generate the tests again.

[rquadling]

HI. Can you rebase to 0.7.x-dev please. This will be the new breaking changes branch (new interface methods or signature changes).

[ghost]

replaced by #926