Skip to content

Rollback --target using migration name#945

Merged
lorenzo merged 2 commits into
cakephp:masterfrom
jradtilbrook:rollback_name
Aug 21, 2017
Merged

Rollback --target using migration name#945
lorenzo merged 2 commits into
cakephp:masterfrom
jradtilbrook:rollback_name

Conversation

@jradtilbrook

Copy link
Copy Markdown

This replaces PR #939.

There is a bug where a any number less than the first migration time stamp (or a typo) supplied to the --target option will cause all migrations to be rolled back. Referring to PR #939, I don't think this should be the case and optimistically the --target option should actually take the migration name, not the migration id (time stamp) because this functionality is basically incorporated into --date.

What this pull request does is add an explicit check for all or 0 passed to the --target option to trigger a rollback of all migrations. This doesn't allow typos or small numbers to incorrectly rollback all.

Going forward, I would either prefer the --target functionality be changed to take the name, or a new option be added to specify the name. Either way, there may be an issue of non-unique migration names.

@jradtilbrook

Copy link
Copy Markdown
Author

This will close #938.

@jradtilbrook

Copy link
Copy Markdown
Author

Looks like HHVM is working again of Travis. My fork of this is passing all tests, but I can't re-trigger a build for this PR. Do you think you will add this functionality change?

if ($found !== false) {
$version = $found;
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check whether or not the version was actually found. If it was not found, throw an error. No one wants to rollback to the first migration just because they made a typo in the cli

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I'll update the tests and add that change in. I'm a little busy for the next few days but I'll see how I go by the weekend

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, thanks for your work!

@jradtilbrook

Copy link
Copy Markdown
Author

I didn't get much time over the weekend to look at this because I had to set up my laptop again.
I see that this PR is for an old branch, can you advise what I should change it to? The Travis config is also old and contains HHVM, looks like I'll need to merge in master?

@lorenzo

lorenzo commented Jul 3, 2017

Copy link
Copy Markdown
Member

@jradtilbrook Try to rebase the branch on top of the current master. It should work without any issues

@tariquesani

Copy link
Copy Markdown

@jradtilbrook Hey! are you still working on this?

@jradtilbrook

Copy link
Copy Markdown
Author

@tariquesani I still plan to but have been quite busy recently. I'm not sure when I'll be able to get some code out for you - if you're wanting something soon maybe its best for someone to take it over instead?

@jradtilbrook jradtilbrook changed the base branch from 0.7.x-dev to master August 20, 2017 03:25
@jradtilbrook

Copy link
Copy Markdown
Author

Alright, so I finally got around to working on this. Rebasing turned out to be too hard so I started again from master. I also updated this PR to point to master as per the CONTRIBUTING doc. Hope its all fine, let me know your thoughts/intentions

@lorenzo

lorenzo commented Aug 21, 2017

Copy link
Copy Markdown
Member

@jradtilbrook thanks for doing this!

@lorenzo lorenzo merged commit c2f9246 into cakephp:master Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants