Doctrine Migrations

[ghost]

This recipe adds doctrine migrations.

[ogizanagi]

Why adding it at the project root? Shouldn't it rather be in src? Or maybe in etc?

[stof]

this should not be at the root. Migrations are code, and so should be in src/.

Btw, this means that the default value of %kernel.root_dir%/DoctrineMigrations will work fine for people putting their kernel in src (which is the case in Flex).

[javiereguiluz]

@Ricknox thanks for proposing this addition. I know this bundle is widely used, it works, it's battle-tested, etc.

But I'm voting 👎 for now because the bundle lacks some basic quality checks needed for the main repo:

• The activity looks very low (last commit Dec 5 2016). Is it because it's mature/finished ... or is it because of lack of contributions?
• The official repo (https://github.com/doctrine/DoctrineMigrationsBundle) doesn't have neither a description, nor a website nor the mandatory GitHub topics for Symfony bundles (https://symfony.com/blog/standardizing-the-github-topics-for-symfony-repositories).
• The README of the repo looks outdated: it mentions "Symfony2" instead of "Symfony", it references to the "deps" file of Symfony 2.0, it alerts about a namespace change that occurred years ago, etc.
• The documentation is OK, but it needs to improve the explanation of the configuration options. They are not explained at all (that's why I don't know for example the purpose of the "name" option).

[javiereguiluz]

What's the purpose of this name option? The name is vague, the default value doesn't help me and it's not explained either in the official docs.

[stof]

I would remove this config:

• the table name you provide here is just repeating the default value, which is fine for most people. So just rely on the default value instead. Adding the config explicitly in projects makes sense only if people are expected to modify it
• the name is almost useless. It is used only for display purposes (and as the bundle does not support having several migration configurations in parallel, using a meaningful name for the migration config does not bring much)

[javiereguiluz]

Then, if possible, let's remove the "name" config option. Thanks!

[javiereguiluz]

The name of this option looks confusing. dir_name would be DoctrineMigrations ... but the value is an absolute path.

[javiereguiluz]

Why do we need both the namespace and the "dir_name"?

[stof]

dir_name defines the path where migrations are looked for when loading them (and the place where they are generated). I agree with you that the name is unfortunate as it is a path.
namespace defines the namespace used when generating new migrations (you never create the boilerplate code of migrations by hand unless you are crazy, as the bundle has CLI commands for that).

And we need to define them explicitly here, because the recipe decided to use non-default values.

[ghost]

A core member of Flex needs to change the requested changes from @javiereguiluz. I accidentally deleted the wrong fork, causing it to say from unknown repository and losing my write permissions to this PR.

[weaverryan]

👍 for this bundle. This is an integral part of the Doctrine workflow.

but ... 👍 for some of the changes that Javier proposed first.

[stof]

The activity looks very low (last commit Dec 5 2016). Is it because it's mature/finished ... or is it because of lack of contributions?

The bundle itself does not implement the business logic but only the glue code. The business logic lives in doctrine/migrations and doctrine/dbal.

And the glue code matured since years (the library too btw), which is why it is not very active.

The official repo (https://github.com/doctrine/DoctrineMigrationsBundle) doesn't have neither a description, nor a website nor the mandatory GitHub topics for Symfony bundles (https://symfony.com/blog/standardizing-the-github-topics-for-symfony-repositories).

I just fixed this by adding the topics and a description

The README of the repo looks outdated: it mentions "Symfony2" instead of "Symfony", it references to the "deps" file of Symfony 2.0, it alerts about a namespace change that occurred years ago, etc.

PRs are welcome to improve this readme (/cc @Ricknox otherwise I might do it myself later)

The documentation is OK, but it needs to improve the explanation of the configuration options. They are not explained at all (that's why I don't know for example the purpose of the "name" option).

[ghost]

@stof Sure, I cannot longer write to this PR. Sorry.

[javiereguiluz]

@stof about the README, there's a pending PR for fixing it: doctrine/DoctrineMigrationsBundle#193

The rest look s great. We'd just need to improve a little bit the docs. This should be easy for anybody using that bundle. We just need to explain better the config options. Thanks!

[stof]

@javiereguiluz the readme PR is now merged. Thanks.

Regarding the config about options, I agree we should document them better. But they are actually all advanced options. For the basic usage of the bundle, you don't need to configure anything (well, you need the dir_name explicitly here, but this is only because the default value in the bundle is not flex-ready yet, which may change in the future if we figure a BC way to change this default value when using flex).

For my work project (and I think also for my previous projects using this bundle, but I haven't checked to confirm it), I don't even have the doctrine_migrations section in my config.yml. I rely on the default config entirely.

[javiereguiluz]

👍 @stof your comment makes sense to me!

Thank you all for your great work on this!

[stof]

@Ricknox we cannot push to the branch either to update the PR, as the repo is gone. So you will need to recreate a new PR from your recreated fork (or try contacting the github support to undo the removal, but I think it is not worth it).

You can get the code from this PR with the following code (my code will assume that you have 2 git remotes configured in your local clone: origin for your re-created fork and upstream for the Symfony repo. Just adapt the names if they are different for you):

# fetch the code of the PR using the special reference exposed by github
git fetch upstream refs/pull/57/head
# create a local branch for this commit
git checkout -b doctrine_migrations FETCH_HEAD
# push this branch to your fork
git push origin doctrine_migrations -u
# Then you can create a new PR

If you have not yet deleted your local clone of the repo, this is even simpler as you probably still have the branch locally and so you don't need to fetch the commit from github and recreate the branch. you just need to push it.

[ghost]

Closing this PR in favor of #62.