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.

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

FEATURE: Doctrine rollback command #2938

Open
wants to merge 3 commits into
base: 8.3
Choose a base branch
from

Conversation

sorenmalling
Copy link
Contributor

With this change, you can now execute

./flow doctrine:rollback

to rollback latest executed migration

Resolves #2937

Upgrade instructions

No changes needed

Review instructions

  • Set up a Flow installation
  • Migrate all tables
  • Use the doctrine:rollback to see only the latest migration is rolledback
  • Further: Run a migration with a older timestamp
  • See that the rollback is solely depended on the executedAt property

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

@sorenmalling
Copy link
Contributor Author

sorenmalling commented Dec 2, 2022

This could be backported to older versions, as it doesn't introduce changes to any concepts

@kdambekalns
Copy link
Member

Hm, given doctrine:migrate did apply multiple migrations, this would only roll back the most recent one, not to the state I had before doctrine:migrate, right? Might not be what users expect from such a command… 🤔

@sorenmalling
Copy link
Contributor Author

Hm, given doctrine:migrate did apply multiple migrations,

True, doctrine:migrate migrates what migration you did not run yet.

this would only roll back the most recent one, not to the state I had before doctrine:migrate, right? Might not be what users expect from such a command… 🤔

doctrine:rollback will rollback one migration at a time - outputting the same, as doctrine:migrate. Rut it multiple times, to get back to whatever states you wan't it to.

@sorenmalling
Copy link
Contributor Author

This is inspired by the rails way of doing this

Rather than tracking down the version number associated with the previous migration you can run:
bin/rails db:rollback

https://edgeguides.rubyonrails.org/active_record_migrations.html#running-specific-migrations

@kdambekalns
Copy link
Member

kdambekalns commented Dec 5, 2022

I see… This idea is not the worst:

If you need to undo several migrations you can provide a STEP parameter:

Even though "step" is a horrible name for that parameter, IMHO. 😇

@sorenmalling
Copy link
Contributor Author

Even though "step" is a horrible name for that parameter, IMHO. 😇

Step is not a part of this implementation of the same reasons :P

Joke aside. I find my self running a migration, and need to rollback. I love the rollback command from rails because of the ease for me as a developer (no copy paste over versions and writing a direction, just a plain and simple command)

@kdambekalns
Copy link
Member

I fully understand. I just wonder if the difference in "default behaviour" is too confusing… You run rollback but may or may not end up at the point you started your migrate from…

@sorenmalling
Copy link
Contributor Author

We never know what we come from (very meta..)

The longer description and headache this rollback features aims to resolve :)

Consider this a simplification of doctrine:executemigration --version --direction down

To you @kdambekalns the following can be considered common knowledge. It's for others to understand the developer headache to rollback a single migration at the time:

To know what we came from, requires us to run doctrine:migrationstatus, read the numbers of migration we are about to migrate (and what versions).

Note: Doctrine commands blocks us from creating a migration, if there is a pending one.

Cases where we might have multiple migrations pending, could be installing a external package. The way to get back to where we came from would be to uninstall the packages and create new migrations in our own packages, or run the migrationexecute down on the versions before removing the packages.

The rollback is a developers helping hand to ease the development and creations of migrations, but also usable, to test migrations if any errors pop up

@kdambekalns
Copy link
Member

Coming back to this, an idea… What about

./flow doctrine:migrate --version -1

to "undo" the last migration? For more, this could work:

./flow doctrine:migrate --version -3

That doesn't "promise" as much as a rollback command… (IMHO)

@sorenmalling
Copy link
Contributor Author

That doesn't "promise" as much as a rollback command… (IMHO)

It delivers what is described in the command description :-)

A different --version format, from a DX point of view seems odd

A new command with a new purpose - It's all about developer experience :)

I just want better DX for my daily tasks.

Prepare for my upcoming doctrine:migrationgenerate --empty patch.

@kdambekalns
Copy link
Member

kdambekalns commented Jan 4, 2023

It delivers what is described in the command description :-)

As if people read those… or even know about flow help… 🙊

Anyway, that reminded me of this old thing I drafted: https://docs.google.com/document/d/1GN10VfnYaSq-PJ3GMxw3L0y6oko6xQw4oVlopHYQgxE/edit?usp=sharing WDYT?

@sorenmalling
Copy link
Contributor Author

It's a good idea to simplify the DX - that is the same goal of this rollback. But at the sime time isolate functionality into a single command and a simply speaking syntax.

Developer Experience - make it easy, to get daily tasks done. We are really lacking behind on that, and we should at least focus on getting nice stuff into the core. I like many of the DX concept from rails and we should really look into bring it to ourself, instead of accepting the current state

And you and I seems to be the only people interested in the topic. So why don't we simply move on this topic?! Get this in, and later find out of the whole doctrine command namespace should be reworked. But lets improve, while we are at it. We seem stucked.

I already have it as a isolated package, I can release that instead, to see if it brings value to other than me.

It can either bring the outcome:

  1. It will never go into the core, because a package exists (untill it's not supported for newer versions)
  2. No one cares, because there is already a command with certain arguments, that brings the same functionality
  3. It will at some point be ported to the core, because it shows value

Off-topic, but deserves a comment

Anyway, that reminded me of this old thing I drafted: https://docs.google.com/document/d/1GN10VfnYaSq-PJ3GMxw3L0y6oko6xQw4oVlopHYQgxE/edit?usp=sharing WDYT?

To comment on the document concept, it reads as mixing things up here. The doctrine:migrate command all of a sudden have a responsibility of generating migrations (empty or not) but also executing it.

I think we should talk about a whole "generate/kickstart" concept, to simplify the kickstart of elements we are using every day

./flow flow:generate package Awesome.Package
./flow flow:generate migration --empty
./flow flow:generate controller --package ... --name Standard

But also

  • NodeTypes
  • Domain Models
  • Fluid Template

and whatever generators we could introduce. And basically move the whole kickstart package concept in to such a pattern

@sorenmalling
Copy link
Contributor Author

And we leave it here

@kdambekalns
Copy link
Member

WTH… I came here to complain about feeling bad because others could also have had a look at this.

Only to realize this works based on "executed at", a fact I had missed all the time! 🤦‍♂️

So to "fully roll back", one can just run the command multiple times, if needed.

@kdambekalns kdambekalns reopened this Jan 30, 2023
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

Looks very good by reading a dozen times by now…

@sorenmalling
Copy link
Contributor Author

Released as own package

https://github.com/wbsply/doctrine-rollback

@PRGfx
Copy link

PRGfx commented Feb 7, 2023

Shouldn't doctrine:migrate prev work for this use-case? When trying it, it seems it doesn't, but in the code I see it tries to resolve a version alias (but maybe it's just migrating upwards, haven't looked too deep). Reading the doctrine documentation I would transfer that there would be a similar mechanic with the flow interface.

You can use version aliases when executing migrations. This is for your convenience so you don't have to always know the version number. The following aliases are available:

  • first - Migrate down to before the first version.
  • prev - Migrate down to before the previous version.
  • next - Migrate up to the next version.
  • latest - Migrate up to the latest version.

@kdambekalns
Copy link
Member

Shouldn't doctrine:migrate prev work for this use-case?

Previous in the "logical order" is not what this does – the change by @sorenmalling takes "time order" into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Rollback latest migration via a single command
5 participants