Only mark migrations as installed after execution

[nickvergessen]

The problem is that if a developer creates a structural error in their migration file,
they will already be marked as executed and an not be rerun.

Happened to @Rello in the analytics app.

[rullzer]

Won't this cause problems if part of it has been applied?

[nickvergessen]

Well currently all are marked as executed even if nothing is executed...

This basically boils down to the "code safe for your step and depend on others", e.g.

• Check if the table you want to drop exists before doing so
  https://github.com/nextcloud/spreed/blob/master/lib/Migration/Version11001Date20210212105406.php

• If you add a column check for the column but fail hard when the table is missing:
  https://github.com/nextcloud/spreed/blob/master/lib/Migration/Version11001Date20210211111527.php#L46

[Rello]

I could add the current case:
in an old migration file, I was using Type-Constants. they were removed with the doctrine update in NC21.
So any new app installation under NC21 failed.
But as the migration file was marked as processed, there was no chance of recovery than asking the user to manually delete a record in the database.

A fix to the migration file was released, but it was not possible to re-execute it

[MorrisJobke]

Makes sense 👍

[MorrisJobke]

@nickvergessen Do we want to backport this?

[nickvergessen]

/backport to stable21