Skip to content

Conversation

@apetitpa
Copy link

@apetitpa apetitpa commented Mar 22, 2017

As suggested in #512, this adds an example of how to use the DoctrineMigrationsBundle to apply database migrations.

apetitpa added 2 commits March 22, 2017 10:39
As suggested in #512, this adds an example of how to use the DoctrineMigrationBundle
Just added more comments and details
@stof
Copy link
Member

stof commented Mar 22, 2017

having a migration starting from an unknown state is a bad idea. This is not the proper usage of the migration library. The first migration should migrate from an empty database so that migration commands are working for everyone.
If you require developers to start by using doctrine:schema:create for the initial setup, migrations are unusable as the dev also need to mark all existing migrations as already migrated at this point (as doctrine:schema:create creates the current schema, not the schema at the time before migrations started to be used)

@javiereguiluz
Copy link
Member

javiereguiluz commented Mar 22, 2017

@stof we may add an example of a migration ... but we don't want to force a useless migration. So we said, imagine that we have a time machine and can go back to the moment before #457 was merged. What would have been the migration created for that change?

The idea is to create the migration that we should have done ... but never did. The migration will be only an example to see it but not to execute it. Nobody will have to execute that to make the app runnable.

@jkufner
Copy link
Contributor

jkufner commented Mar 22, 2017

@stof I agree the migrations should go from empty database to current state. However, I don't see reason why they should not be already applied when a developer clones this repo (except some meaningless migration to serve as example). List of applied migrations is in the database, so it should not cause any problem since the database is distributed with the source code.

@apetitpa
Copy link
Author

@stof I added the migration number to the migration_versions table in both sqlite databases so that the command php bin/console doctrine:migrations:status shows that the database is already at the latest version. Therefore the migration does not need to be applied.

{
$this->addSql('ALTER TABLE symfony_demo_user ADD fullName VARCHAR(255) DEFAULT NULL');
$this->addSql('UPDATE symfony_demo_user SET fullName = username');
$this->addSql('ALTER TABLE symfony_demo_user CHANGE fullName fullName VARCHAR(255) NOT NULL');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apetitpa, this syntax is not supported by sqlite.

See https://www.sqlite.org/lang_altertable.html

Copy link
Author

@apetitpa apetitpa Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'm afraid this is valid for mysql databases only. Do you know how can I achieve the same with sqlite ? From what I read it seems that it is not possible to alter columns definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apetitpa, try to use any string ("Unknown" for example) as a default value:

$this->addSql('ALTER TABLE symfony_demo_user ADD fullName VARCHAR(255) NOT NULL DEFAULT "Unknown"');

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@voronkovich thank you for your help

@apetitpa
Copy link
Author

ping @stof @javiereguiluz what about this PR ?

@stof
Copy link
Member

stof commented May 10, 2017

I'm actually -1 on this PR, because it shows an invalid usage of doctrine/migrations.

The content of app/DoctrineMigrations assumes that you will do the initial database creation with doctrine:schema:create (or doctrine:schema:update) and then use migrations to apply later changes.
But this works only if you run the schema command with a codebase before the introduction of migrations (otherwise, the DB already has the changes performed by the migration, but the migration is not known as being executed). This makes it impossible to build the DB from an empty database properly.

The right usage of doctrine/migrations is to write the first migration as a migration from the empty database to the needed one (and then never using doctrine:schema:create or doctrine:schema:update to apply changes to the DB).
So either the demo switched to migrations fully, or it does not show them. But showing them the wrong way is the worst option.

@Tobion
Copy link
Contributor

Tobion commented May 18, 2017

Agree with stof

@apetitpa
Copy link
Author

apetitpa commented May 18, 2017

I'll wait for a final word from @javiereguiluz and @jkufner and then I'll close the PR

@javiereguiluz
Copy link
Member

@apetitpa I can't really add anything here because I don't have much experience with migrations. But if Christophe and Tobias agree that this is the wrong way to use/teach this feature, we can't merge this.

It's sad because you spent some time preparing this pull request, but this is something that sometimes happen when contributing to open source projects. I'm sorry.

@apetitpa
Copy link
Author

apetitpa commented May 18, 2017

@javiereguiluz it's ok don't worry, as you said it happens sometimes but it is part of the game and it's for the greater good. Thanks to @stof and @Tobion, Symfony beginners won't be confused about migrations and I think this is the most important point. I close this PR since it won't be merged. Thank you for your time everyone.

@apetitpa apetitpa closed this May 18, 2017
@apetitpa apetitpa deleted the migration-example-#512 branch May 18, 2017 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants