-
Notifications
You must be signed in to change notification settings - Fork 2.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
Change OC_Migrate from MDB2 to Doctrine #6005
Conversation
Sweet!! Thanks a lot @bartv2 👍 Haven't tested |
@bartv2 can we add a unit test? thx |
ping |
@bartv2 the migration now runs, but I can't see that the migration.db sqlite database is actually generated? |
@tomneedham what do you mean, here https://github.com/owncloud/core/pull/6005/files#diff-6dcfe22b2c6accaeb57bd13238358382R456 the database is created. here https://github.com/owncloud/core/pull/6005/files#diff-6dcfe22b2c6accaeb57bd13238358382R488 the tables are created. |
@bartv2 In the resulting zip file that is sent to the user, there is no migration.db present. |
@bartv2 Can you take a look at this? The export runs ok but I don't see a migration.db generated in the export zip. |
@@ -1 +1 @@ | |||
Subproject commit 42efd966284debadf83b761367e529bc45f806d6 | |||
Subproject commit daf25846dbb8a326f4801dc3310cd2b8ab1f84b9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite sure that the tests fail because of this commit. We had the same problem with the xsendfile pull request. Why do we change the 3rdparty reference in this pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartv2 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit without this also failes the ci tests. The only changes in those commits are:
- add php-cloudfiles
- something with CommentOnColumnSQL
- remove MDB2
thus, not very critical
I checked, only the user export has a migration.db which is also what the code suggests. |
Test failed. |
@bartv2 which applications did you have installed that use migration of db content? |
@bartv We need to change this code to doctrine too: Line 467 in 8f0c56c
When migrating it tries to recreate the tables in the new instance (which may already be there). I get the following for example when migrating the calendar to an instance with the calendar already installed: An exception occurred while executing 'CREATE TABLE "clndr_objects" ("id" INTEGER NOT NULL, "calendarid" INTEGER DEFAULT 0 NOT NULL, "objecttype" VARCHAR(40) DEFAULT '' NOT NULL, "startdate" DATETIME DEFAULT '1970-01-01 00:00:00', "enddate" DATETIME DEFAULT '1970-01-01 00:00:00', "repeating" INTEGER DEFAULT 0, "summary" VARCHAR(255) DEFAULT NULL, "calendardata" CLOB DEFAULT NULL, "uri" VARCHAR(255) DEFAULT NULL, "lastmodified" INTEGER DEFAULT 0, PRIMARY KEY("id"))': SQLSTATE[HY000]: General error: 1 table "clndr_objects" already exists |
@tomneedham createAppTables is only used when exporting, so not sure how you get that error |
Test passed. |
👍 Just need to work on unit tests. Not going to be easy. |
Solves #5388
@tomneedham @DeepDiver1975