Make diff/generate commands more extensible

[garex]

To generate Phinx migrations from Doctrine's schema diff I implemented this simple bridge bundle: https://packagist.org/packages/ekapusta/doctrine-phinx-bridge-bundle

But ugly part in it is that I need to reinvent the bicycle over and over as DiffCommand has many private methods and uses direct calls for most operations.

Just compare https://github.com/doctrine/migrations/blob/master/lib/Doctrine/DBAL/Migrations/Tools/Console/Command/DiffCommand.php and https://github.com/doctrine/migrations/blob/master/lib/Doctrine/DBAL/Migrations/Tools/Console/Command/GenerateCommand.php with https://github.com/ekapusta/doctrine-phinx-bridge-bundle/blob/develop/Command/DiffCommand.php

It would be good if:

• template and classname will be configurable
• both schema filtering will be in single simple method
• filename will be calculable
• buildCodeFromSql's sprintf template will be configurable

[stof]

Well, your project is not using the doctrine/migrations system to handle migrations, so why trying to reuse the DiffCommand while subverting it to a different job ?
Getting the SQL queries does not involve using doctrine/migrations as all the Schema system is part of DBAL itself.
Regarding what you have in your own DiffCommand, the removal of the migration table is specific to doctrine/migrations (Phinx migration might have a similar need, but it is something you should kept separate anyway as there is no guarantee that they will keep working the same in the future).

Adding extension points in the commands to make them reusable with a totally different system is a huge maintenance cost, because they would forbid us to make the architecture of commands evolve (as each place where we have an extension point would be locked)

[garex]

@stof,

is not using the doctrine/migrations system to handle migrations,

Because it's not extensible now. They both looks so similar: generates files by template with some class inside and do raw SQL queries. I think this model could be easily extensible.

the removal of the migration table is specific to doctrine/migrations

phinx also use same model: table in target DB to keep history of migrations.

Adding extension points in the commands to make them reusable with a totally different system is a huge maintenance cost

https://en.wikipedia.org/wiki/Open/closed_principle ?

[mnvx]

I agree with @garex.

Would be great to extend these classes to specify migration name when I create migration. For example, with next syntax:

php bin/console doctrine:migrations:diff --name=CreateDB

And I want have as result file with migration named Version2016010113331_CreateDB.php with class name Version2016010113331_CreateDB.

Currently it is possible only with fork. It is not very good.

[garex]

@mnvx I think in next 100 years this will be possible ))

See doctrine's PR as an example (btw this is real bug fix). doctrine/dbal#2490

[chrisguitarguy]

See doctrine's PR as an example (btw this is real bug fix). doctrine/dbal#2490

There's some reasoning why that PR wasn't merged: doctrine/dbal#2490 (comment)

template and classname will be configurable

This is done in 1.6+

@alcaeus or @mikeSimonson I think this one can be closed unless any of the remaining extensibility suggestions are worth pursing.

[alcaeus]

template and classname will be configurable

Template is configurable starting with 1.6.0. Class name is fixed as that determines the version.

both schema filtering will be in single simple method

filename will be calculable

File name directly relates to the class name, which in turns leads to the reasoning in your first point.

buildCodeFromSql's sprintf template will be configurable

What for? There's just a bunch of addSql calls containing SQL statements generated by the DBAL and a sanity check that really shouldn't be removed. This is sensible and IMO doesn't warrant the overhead for overriding that template, as you can just change the generated code once the class has been created.

As per @chrisguitarguy, I'm closing this for now. Please feel free to follow up if you feel I haven't considered all the angles.

[garex]

@alcaeus if you think this can be implemented without tons of copy-paste, then close it:

Copy-paste stuff: https://github.com/ekapusta/doctrine-phinx-bridge-bundle/blob/develop/Command/DiffCommand.php

[alcaeus]

This libraries' job is not to provide an extensible framework for generating and running migrations - it's a library for generating and running migrations in its own context. If you want to do it for Phinx, write your own. Just because your library generates code and this library generates code doesn't mean that they need to share an ancestor.

[garex]

@alcaeus making extensible code is a hard work than just writing god-mode code for single use. I dont' undertand position of avoiding any extensibility just because of "one-goal".

[mikeSimonson]

@garex You will understand when you will have to maintain it.

Have a good day