The argument for marking it breaking if it contains a database migration is that it requires you to run them when updating the package. This has been done in the past (example), but since it doesn’t necessarily mean changing any public marked API it’s not the best way to do it to clearly distinguish between the two.
So question is if we should just ignore that and expect everyone using master or updating patch versions to run doctrine:migrate as part of that process?
An alternative would be to mark changes that require you to do something manually in another way. E.g. with [!] or [~].
But doctrine:migrate should be a part of everyone’s deployment process (with Surf or any other tool) in any case?
I would not make it equal to changes that require core migrations or manual code adjustments.
giving this some more thought I would say we need to clear up a few questions together with this:
How do we expect people to do updates?
I would usually expect they test the update locally and then run a deployment when they know it works. I wouldn’t want to run DB migrations on my live DB without knowing it works beforehand, so IMHO such changes need to be marked in someway so I know that I need to test this update before doing it live.
How long will a migration run?
Most migrations are very fast but depending on the migration this is a break point in a deployment. When you run the migration the site might become unavailable until the new code is deployed as well (thinking of surf) so if the migration takes some time (scales badly or whatever) this is very important to know and needs to be marked.
Are migrations allowed in patch level releases?
I think this is something we need to consider for the future especially for an LTS release. I would strongly suggest to think about not allowing migrations in patch level releases anymore for the point mentioned in question 1 that you usually would want to test the update beforehand and I see it as unreasonable to expect people to test any patch version update (think security updates, those should be rolled out fast, any “blocker” that we introduce will slow that down). If we decide to allow migrations in minor releases, they should definitely be marked so that I know if a patch release needs to be tested before rolling it out…
avoid adding migrations to patch-level releases when ever possible – only if it’s really there to fix a nasty bug
we expect (and communicate) that doctrine:migrate is run as part of an upgrade from one version to another
migrations (and later on, even node migrations) must not require manual steps but should be possible to integrate into an automated deployment
a migration is breaking if the new database schema would cause errors when being run with the previous code version
So, ideally, in minor releases we only allow migrations where the resulting database schema works without errors with any other patch level release in that minor branch. So, adding a property, for example, in 2.3.0 would be fine, as even 2.2.82 would not produce errors but just ignore it. Removing a property would only be allowed in 3.0.0.
I’m in favor or Robert’s proposal as it seems to make sense to me, too. No migrations needed on patch-level updates (except it’s clearly communicated in the release notes as an exception and, well, really has to be).
For minor and major version bumps I see no issue if migrations have to be executed.
As it’s in line with the stuff I mentioned I agree to robert there.
But he still doesn’t answer when and how to mark changes with migrations…
I would suggest to always mark them and for the sake of simplicity just with the breaking sign as so far.
The commit message should then mention that this change was marked breaking to notify about the included migration.
Exactly @christianm, the question is not about migrations in minor versions. Nobody wants that unless absolutely necessary, so should be avoided and not allowed in general which is already the case. But the matter of the fact is that they do happen as we can tell from the past, so having a way to mark it would be useful. And as said it happens often for those using master and on rare occasions for patch releases.
It’s fine to mark commits containing migrations as “requires your attention” somehow. I don’t really care how that is done in detail, could be just using our [!!!] prefix or something else. What speaks a bit against introducing [DB] is that subject lines can become a bit too complicated when we start combining different labels.
Another possibility would be to include some keyword into the body which can be searched for when creating change logs. But really, I would just pick any solution, for example the [!!!] which is already established.
Yep great, so as this is a limited impact decision that virtually can be changed any time I would say hereby we decided to mark changes with a DB migration with [!!!] explaining in the commit message that this is marked because it has a DB migration.
