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

doctrine migrations #14851

Merged
merged 5 commits into from
Dec 12, 2016
Merged

doctrine migrations #14851

merged 5 commits into from
Dec 12, 2016

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Mar 12, 2015

requires 3rdparty PR owncloud-archive/3rdparty#279
refs #13820

TODO:

@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2015

Is this still a topic for 8.2, or later ?

@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2015

(exciting topic)

@DeepDiver1975
Copy link
Member Author

I would love to bring this into 8.2 giving apps the opportunity to use it. With e.g. 8.3 we can think about using it for core as well.

@PVince81
Copy link
Contributor

PVince81 commented Sep 1, 2015

@DeepDiver1975 needs rebase, and there are still some items in the TODO list.

Move to 9.0 ?

@DeepDiver1975 DeepDiver1975 modified the milestones: 9.0-next, 8.2-current Sep 18, 2015
@DeepDiver1975 DeepDiver1975 modified the milestones: 9.1-next, 9.0-current Nov 24, 2015
@DeepDiver1975
Copy link
Member Author

moving this to 9.1 - requires architectural decision by @owncloud/architecture

@karlitschek karlitschek modified the milestones: 9.2-next, 9.1-current Mar 31, 2016
@PVince81
Copy link
Contributor

Do we still need this ? I remember we discussed something about using repair steps / migration steps instead. @DeepDiver1975

@DeepDiver1975
Copy link
Member Author

We still need this as a replacment for the current DB XML magic

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

Maybe auto-create the migrations directory ?

Steps:

  1. Check out the "customgroups" app (I have an apps folder in "apps3" for that, glad to see it detects that)
  2. Run the command to create migrations
± % sudo -uwwwrun ./occ migrations:generate customgroups
vincent's password:
Loading configuration from the integration code of your framework (setter).

                                                                                                         
  [InvalidArgumentException]                                                                             
  Migrations directory "/srv/www/htdocs/owncloud/apps3/customgroups/appinfo/Migrations" does not exist.  
                                                                                                         

migrations:generate [--editor-cmd [EDITOR-CMD]] [--configuration [CONFIGURATION]] [--db-configuration [DB-CONFIGURATION]] [--] <app>

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

also the target dir "customgroups/appinfo/Migrations" doesn't seem right, usually we put all classes into "lib" so I'd expect it to put to either "customgroups/appinfo/lib/Migrations" or "customgroups/appinfo/lib/AppInfo/Migrations" (and auto-create the folder if it doesn't exist)

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

I created the folder manually for now, uh oh:

~/work/workspace/owncloud [poc-doctrine-migrations *]
± % sudo -uwwwrun ./occ migrations:generate customgroups
vincent's password:
Sorry, try again.
vincent's password:
Loading configuration from the integration code of your framework (setter).
Generated new migration class to "/srv/www/htdocs/owncloud/apps3/customgroups/appinfo/Migrations/Version20161209105453.php"

~/work/workspace/owncloud [poc-doctrine-migrations *]
± % ll srv/www/htdocs/owncloud/apps3/customgroups/appinfo/Migrations/Version20161209105453.php
ls: cannot access 'srv/www/htdocs/owncloud/apps3/customgroups/appinfo/Migrations/Version20161209105453.php': No such file or directory

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

I thought that maybe I should enable the app first ? (doesn't make sense, but let's try it)

± % sudo -uwwwrun ./occ app:enable customgroups         

                                                 
  [Doctrine\DBAL\Migrations\MigrationException]  
  Could not find any migrations to execute.      
                                                 

app:enable [-g|--groups GROUPS] [--] <app-id>

@DeepDiver1975
Copy link
Member Author

the folder is generated in here: https://github.com/owncloud/core/pull/14851/files#diff-333567435f8bef9975c29c5e73ab7dd0R50 - strange ... let me debug

@DeepDiver1975
Copy link
Member Author

Generating migrations for an disabled app works for me

$ ./occ migrations:generate testapp
Loading configuration from the integration code of your framework (setter).
Generated new migration class to "/home/deepdiver/Development/ownCloud/master-autotest/apps/testapp/appinfo/Migrations/Version20161209113613.php"
$ ll apps/testapp/appinfo/Migrations/Version20161209113613.php 
-rw-r--r-- 1 deepdiver deepdiver 609 Dez  9 12:36 apps/testapp/appinfo/Migrations/Version20161209113613.php

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

Ok then, I'll debug this myself

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

It was a permissions issue: my workspace is under my own user but OCC needs to run as web server user. So web server user has no perms to create stuff in this app...

I added a little check to see if mkdir failed, that's all I can do: e1699e8
If the dir exists but the migration file could not be created, the DBAL base class doesn't catch it and shows a success message... nothing we can do. It doesn't even give access to the path of the migration.

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

ok, had some success after many struggles: initial DB creation works here owncloud/customgroups#11

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

After enabling customgroups on this branch owncloud/customgroups#11 the tables were created.
Then I created a second migration with higher timestamp number that creates another dummy table, for testing.
Then I increased the version in info.xml.
Run occ upgrade

Expected: new table created
Actual: no new tables

@DeepDiver1975 is that the correct way ? The table "oc_customgroups_migration_versions" only contains the first initial migration, so Doctrine should automatically know that it should run the second one.

@DeepDiver1975
Copy link
Member Author

Yes. The new step should be executed.

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

I put a breakpoint in MigrationService::migrate() and it does not go there on upgrade, so something is stuck somewhere.

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

Uh oh...

		if (isset($appData['use-migrations']) && $appData['use-migrations'] === true) {

in updateApp returned false... some info.xml parsing bug possibly.

I'll look into it...

@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

eval result:

  $appData['use-migrations']     = (string[4]) 'true';

'true' === true

FACEPHPALM

A bool is not always a bool...
@PVince81
Copy link
Contributor

PVince81 commented Dec 9, 2016

Added 027979f

Works now 👍

@PVince81 PVince81 merged commit 3f23d15 into master Dec 12, 2016
@PVince81 PVince81 deleted the poc-doctrine-migrations branch December 12, 2016 08:21
@lock
Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants