Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle deleted migration classes #795 #808

Merged
merged 1 commit into from
Apr 17, 2019
Merged

Handle deleted migration classes #795 #808

merged 1 commit into from
Apr 17, 2019

Conversation

jschaedl
Copy link
Contributor

@jschaedl jschaedl commented Apr 5, 2019

Q A
Type feature
BC Break no
Fixed issues #795

Summary

@jschaedl jschaedl mentioned this pull request Apr 5, 2019
Closed
@jschaedl jschaedl force-pushed the patch-795-2 branch 2 times, most recently from f6b2b5f to b842395 Compare April 5, 2019 11:54
jwage
jwage reviewed Apr 5, 2019
View reviewed changes
Copy link
Member

@jwage jwage left a comment

Choose a reason for hiding this comment

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

Looks good. Can you move that delete functionality to a service and add a unit test for it?

@jwage jwage added this to the 2.1.0 milestone Apr 5, 2019
@jwage jwage self-assigned this Apr 5, 2019
@jschaedl
Copy link
Contributor Author

jschaedl commented Apr 7, 2019

Looks good. Can you move that delete functionality to a service and add a unit test for it?

Sure I can do that. But not sure if we need a whole new Service class like MigrationVersionRemover 🤔

As an alternative we could add a new method to the MigrationRepository and name it deleteMigrationVersionFromDatabase(string $version): void.

WDYT?

@jwage
Copy link
Member

jwage commented Apr 7, 2019

MigrationRepository sounds good.

@jwage
Copy link
Member

jwage commented Apr 7, 2019

Looks good. Can you squash? and then I think we are ready to merge.

@jschaedl
Copy link
Contributor Author

@jwage I squashed, so ready to merge :-)

@jwage
Copy link
Member

jwage commented Apr 10, 2019 •
edited
Loading

Looks like CS is failing. Run vendor/bin/phpcbf to fix. https://travis-ci.org/doctrine/migrations/jobs/516964516

@jschaedl
Copy link
Contributor Author

@jwage I think we are ready to merge. :-)

@jwage jwage merged commit 7daeea2 into doctrine:master Apr 17, 2019
@jwage
Copy link
Member

jwage commented Apr 17, 2019

Thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwage jwage jwage approved these changes

Assignees

@jwage jwage

Labels
Enhancement
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jschaedl @jwage