-
Notifications
You must be signed in to change notification settings - Fork 168
Provide developer documentation on migrations #2742
Conversation
@DeepDiver1975, thanks for your PR! By analyzing the history of the files in this pull request, we identified @RealRancor to be a potential reviewer. |
|
||
|
||
As soon as migrations are enabled any existing :file:`appinfo/database.xml` will be ignored on upgrade of the app. | ||
Instead all migration step which not yet have been executed will be executed. |
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.
"migration steps" (plural)
As soon as migrations are enabled any existing :file:`appinfo/database.xml` will be ignored on upgrade of the app. | ||
Instead all migration step which not yet have been executed will be executed. | ||
|
||
How to develop a migration? |
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.
Remove question mark
|
||
<?php | ||
|
||
namespace OCA\dav\Migrations; |
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.
Replace "dav" with "MyAppName" ?
} | ||
|
||
Within the up() method the schema changes are to be added by working on the `Class Schema <http://www.doctrine-project.org/api/dbal/2.5/class-Doctrine.DBAL.Schema.Schema.html>`_. | ||
Within the down() method the schema changes are to be reverted. |
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.
is it mandatory to implement down()
? What if the changes are not reversible like some repairs or deleting a column ?
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.
It is only mandatory if we ever allow doangrading 🙈
Within the up() method the schema changes are to be added by working on the `Class Schema <http://www.doctrine-project.org/api/dbal/2.5/class-Doctrine.DBAL.Schema.Schema.html>`_. | ||
Within the down() method the schema changes are to be reverted. | ||
|
||
3. Test your migration step by executing |
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.
Unit tests for migrations, does that make sense / is it possible / wanted ?
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.
because all steps are executed upon installation there is no real need from my pov
|
||
./occ migrations:execute --up dav 20161130090952 | ||
|
||
In order to revert the migration you an run |
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.
s/an/can/
|
||
./occ migrations:execute --down dav 20161130090952 | ||
|
||
By executing up and down can explicitly apply the changes and roll them back. |
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.
s/can/you can/
4. Bring the migration live | ||
|
||
For the real app upgrade process the app version has to be increased to trigger the migrations. | ||
This will apply all steps which have not yet been executed. |
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.
How does Doctrine know which migrations to run for which versions ?
How does Doctrine find the migration classes, do they all have to be in a specific package ?
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.
How does Doctrine know which migrations to run for which versions ?
there is a table for each app holding the list of all already executed steps - no idea if we should put this in the documentation
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.
Might be a good idea to add it, along with the relevant documentation, so that if anyone is new to Doctrine, they can quickly get up to speed.
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.
Well - this is an implementation detail of doctrine migrations from my pov.
There are also not docs available upstream about these internals.
Thanks for the docs, that clarifies a bit. See comments. |
regular php classloader mechanism |
You said there is a table to track which migrations have run already. But is there a table of the ones that haven't ? Or is the classloader trying to find all migration classes within the app's namespace in any namespace and sort them by version ? |
There is one namespace for each app \OCA\MyAppName\Migrations. |
@DeepDiver1975 can you add your answers to my questions inside the documentation ? Because other people will also wonder... |
3b061d3
to
f281ebe
Compare
@PVince81 please have another look - thx |
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.
Looks better.
A few minor comments and this is good to go!
|
||
|
||
As soon as migrations are enabled any existing :file:`appinfo/database.xml` will be ignored on upgrade of the app. | ||
Instead all migration steps which not yet have been executed will be executed. |
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.
Instead,
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.
which have not yet been executed
How are migrations detected and executed | ||
======================================== | ||
|
||
For each app there will be a table holding the list of migrations which have been executed. |
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.
which have been executed during past upgrades
(the precision makes it even clearer)
======================================== | ||
|
||
For each app there will be a table holding the list of migrations which have been executed. | ||
This list is compared to all migration classes in the app's migration folder :file:`appinfo/migrations`. |
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.
... and the migrations that are not present in the table will be executed in the order of the class names alphabetically
(or revision numbers ?)
|
||
ownCloud uses `Doctrine Migrations <http://www.doctrine-project.org/projects/migrations.html>`_ to manage any kind of database changes. | ||
|
||
Any apps which wants to use this mechanism has to enable it in :file:`appinfo/info.xml` by adding |
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.
Any apps which want to use this mechanism have to enable it in
or
Any app which wants to use this mechanism has to enable it in
|
||
By executing up and down you can explicitly apply the changes and roll them back - as long as down() is properly implemented. | ||
|
||
Because all migration steps will be executed upon installation there is no explicit need on unit tests. |
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.
s/on unit tests/for unit tests/
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.
Just a couple of bits of English grammar.
f281ebe
to
c8f236a
Compare
Thanks for your feedback @phil-davis @PVince81 |
👍 |
@settermjd please review and merge - thx |
Sorry, but I have another question: when creating an app from scratch, does the developer need to develop an initial migration that creates the database ? If yes, it would be good to add that to the docs as well. |
the initial migration will create the table. $prefix = $this->connection->getPrefix();
$table = $schema->createTable("{$prefix}activity");
$table->addColumn(...);
$table->addIndex(...);
$table->addForeignKeyConstraint(...); |
Thanks, can you add a note in the documentation that we need an initial migration instead of database.xml to create the tables initially ? The reason I was confused is because this was missing. |
(no need for the code details, I think the Doctrine docs have examples) |
Alternative: provide a good example of what a fully implemented migration looks like. |
Ok, found an example here: https://akrabat.com/using-doctrine-migrations-outside-of-doctrine-orm-or-symfony/ |
Note for future schema porters: if you see "text" in the MDB2 schema, don't use it! Use "string" instead in the migration definition. Because "text" will explode with Mysql if you set an index on it, see owncloud/customgroups#11 (comment) |
|
||
|
||
As soon as migrations are enabled any existing :file:`appinfo/database.xml` will be ignored on upgrade of the app. | ||
Instead, all migration steps which have not yet been executed will be executed. |
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.
@DeepDiver1975 this doesn't fully make sense. The previous sentence says that they'll be ignored, yet this one says that any outstanding ones will be executed. Can you clarify pls?
This commit revises the content and separates out the larger code examples.
I made a few revisions to the file. If no one objects, then I'm happy to merge the PR. |
:linenos: | ||
|
||
To update the tables used by the application, update database.xml and increase | ||
the application version number in :file:`appinfo/info.xml` to trigger an update. |
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 think this is not true any more. Changes in this file are not used any more except for the initial creation.
Or at least that's why I observed when updating from stable9.1 to master, the code would not go there.
@DeepDiver1975 please confirm
*/ | ||
public function up(Schema $schema) | ||
{ | ||
// this up() migration is auto-generated, please modify it to your needs |
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 suggest adding a real example here which would ease the life of many.
Have a look at https://github.com/owncloud/core/pull/26923/files#diff-f853a252a02574b7da2d70ca20e06a6eR18 for table creation (ignore hasTable
for now).
Note that we also need to clarify whether we want developers to always explicitly specify the correct collation as it's not automatic. See this issue: owncloud/core#26925
I'll try and clarify this with @DeepDiver1975 next week
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'll hold off on making any changes until you've gotten further clarification.
We need to rework this - we had to remove doctrine/migrations .... |
@DeepDiver1975 what's being used in place of it? |
@settermjd nothing yet... work in progress |
@PVince81 ok. thanks for the update. |
@DeepDiver1975 time to update the docs ? |
@PVince81 @DeepDiver1975 can this be closed in deference to #2862? |
fixes #2726
@PVince81 mind reviewing? THX