-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Migrations #5155
Comments
seconded 👍 |
👍
👍 |
I like the approach. Sounds clean! 🚀 👍 |
Found a little issue. However when you have multiple major branches, or introduce a migration in an older version, it will skip all "older" migrations from the newer major branch are not executed anymore. So I propose to add a version in between which is given by the developer: This allows us to to correctly sort them. The thing is the engine only can sort by name, and doesn't sort by branch or whatever |
Doctrine vs. ownCloud
I looked into Doctrine and ownCloud Migrations today, to see what would fit better with our needs. I saw that the difference is quite minimal:
The code they come with is also rather minimal, since most of the PRs are the actual migrations.
I would now propose to go with ownCloud migration, only with two little changes:
Change 1 - Single type only
They have 3 migration types: schema, sql and simple: https://github.com/owncloud/core/pull/27041/files#diff-333567435f8bef9975c29c5e73ab7dd0R372
I would merge them all into one kind with:
Any of those can be empty. But it allows you e.g. to store data inmemory instead of temp dir, change your db schema and then restore the data again, like we already did it in https://github.com/nextcloud/server/blob/master/apps/twofactor_backupcodes/lib/Migration/CopyEntriesFromOldTable.php
If you dont change the schema, you just leave it unchanged and only change your data in pre/post.
Change 2 - Location
They have the migration files in appinfo/Migrations/ and load them via require_once. I would chose lib/Migration so we have normal autoloading.
cc @LukasReschke @rullzer
The text was updated successfully, but these errors were encountered: