yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

migrations load dependencies like assets do #8202

Closed Faryshta closed 8 years ago

Faryshta commented 9 years ago

Lets see how assets load dependencies.

class AppAsset extends AssetBundle
{
    public $depends = [
        'yii\web\YiiAsset',
        'yii\bootstrap\BootstrapAsset',
    ];
}

This allows a tree-like loading of dependencies each one calling the resources and executing the actions it requires. Since each asset is recognized by the unique full class name (with the namespace) then each asset is unique and can be autoloaded without issues.

The way the migrations do it

class m150427_071254_user extends Migration
{
    // no more code
}

This approach uses the moment the migraiton was created to execute each dependency in chronological order, this implies several differences such as

1) Classes can't be autoloaded

2) There can't be a hard dependency that gets forced to be executed first

3) There is no way to know which migration actually depends on another or force a hard dependency for example asking to execute a migration user_profile that add columns to the user table.

4) a path is always required to be provided when executing the migrations.

5) When executing migrate/down command there is no way to know which other migrations will be affected.

So i propose to have a remaking of this for 2.1 to something similar to


namespace Acme\Sihpments\migrations;

use yii\db\Migration;

class ShipmentLog extends Migration
{
    $depends = [
         'Acme\Shipments\migration\Shipment'
    ];
}

The migration ShipmentLog creates a shipment_log table for all the stages and dates of a shipment, The table requires a foreign key to link it to the shipment table defined on the Shipment migration.

This method is specially got for plugins or modules that extend the functionality of an existing table, example if i want to add columns to the user table defined on the yii migrations.

klimov-paul commented 9 years ago

This does not make sense for me.

This approach uses the moment the migraiton was created to execute each dependency in chronological order

Yes, migration is a kind of version control system for your database. Each migration represent a particular state of the DB structure evolution. Of course it could be said that each particular migration may have a 'parent' and a 'child', but there is no sense of explicitely bind them as they are already ordred using timing principle.

1) Classes can't be autoloaded

Just why do you need that? Class autoloading make sense in case you always know explicit name of the class you want to get. For the migration it does not work this way. Each migration has a unique name composed from time string. In order to find any new migration we need to scan thier source directory one way or another. None will tell migrate command which class name(s) are holding new migrations.

2) There can't be a hard dependency that gets forced to be executed first

Why do you need that? This does not make any sense at all. Migration are always executed in timing order, ensuring composed database state will be stable. Why any dependency should be? Even if it will be, how do you suggest to revert migration with dependencies? Revert all migrations, which this one depends on? And what if other migrations depends on those?

4) a path is always required to be provided when executing the migrations.

As I said there is no way to find the new migrations unless scan thier source directory. Of course we can use a namespace for the migration classes and then may attempt to resolve migration path using Yii::getAlias(), but this will cost more trouble then benefit.

5) When executing migrate/down command there is no way to know which other migrations will be affected.

What does this mean? When you run migrate/down it shows you EXACT names of the migrations, which will be reverted.

I suppose you mistake migration mechanism for something else. Maybe you should start from explanation of the problem(s), which you attempt ot resolve?

xskif commented 9 years ago

I think @Faryshta want to separate app into modules with ready to use migrations. And reuse modules in other apps, which require some control mechanism of migrations. Example:

  1. Start new app.
  2. add module User.
  3. Use user migration.
  4. add module Post.
  5. Use post migration.
  6. Write new module - Comments.
  7. Write Comments migration. and use it
  8. Something happens and i need to remove the Post module so i need to revert concrete migration, but i can't, because migrate/down reverts Comments migration.
  9. FAIL =)
klimov-paul commented 9 years ago

That is why migrations are allowed to be separated by paths. If you have several independent migration flows (like modules) you can separate them using different paths. Module is meant to be an independent entity. If it holds some internal migrations they should be independed.

xskif commented 9 years ago

@klimov-paul , i knew that. Can i revert the concrete migration for the concrete module if i have an independent migrations for each module?

klimov-paul commented 9 years ago

So, you wish to have an ability to revert migration under particular path, like following:

yii migrate/down 1 --path='@mymodule/migrations'

Currently path is not stored in the migration table. However you can use different migration tables for different path:

yii migrate/down 1 --path='@mymodule/migrations' --migrationTable='MyModuleMigrations'
Faryshta commented 9 years ago

I think the point here is that migrations are not linear in the real world.

For example i can write a migration today based on the yii2 rbac migration and i will just add a few functionalities.

Tomorrow a new and improved rbac is published by a third person, so i wanna change the dependency that my plugin no longer uses the yii2 migration and uses the other one instead... even if my migration was written before.

Another much more realistic case would be that the client wanna drop a functionality, add a new functionality, revert to a previous way to do the bussiness logic.

my options would be 1) screw the migrations and do it by hand. right now there is no way to know which migrations depends on each other

or 2) check by hand which migration affect which.

the current system discourage the use for migration for things like extensions, modules and plugins which want to add functionality to an existing migration. RBAC is a great example.

Now imagine with more complex systems like e-commerce. I can write my selling platform using a shopping cart from a 3rd developer. I would need to add steps on my documentation for each migration the shopping cart has.

now imagine that besides shopping cart i need product list, categories, tagging for each product, recomendations, alerts for product existances, etc, etc, etc. Each of them provided by different vendors.

well i can either duplicate the code or make a huge list of all the migration my platform will require to execute.

Faryshta commented 8 years ago

can we talk about this again since we are already preparing 2.1?

I can work on a pr but it will take me a lot of time and effort so I wanna know if I won't waste my time

samdark commented 8 years ago

There are multiple points and I agree with some:

  1. Migration class names could be changed to be PSR-compliant.
  2. Migrations could be namespaced. That would not save you from specifying path but may make it easier via specifying namespaces instead and relying on autoloading.
  3. Child-parent relationships of migrations... that's interesting but let's imagine a situation: we have app migrations and we need to add some third-party module migrations. How to do that? Module migrations, obviously, could not rely on app migrations.
Faryshta commented 8 years ago

we have app migrations and we need to add some third-party module migrations. How to do that?

We can let composer deal with it.


MyMigration extends BaseMigration
{
    public $extends = [
        \Third\Party\ShoppingCartMigration::class,
        \console\migrations\Init::class
    ];
}

we install the third party migrations using composer and let composer load the class

Faryshta commented 8 years ago

@yiisoft/core-developers any update with this? can I start working on this for 2.1?

samdark commented 8 years ago

I don't know. That looks kinda complicated and fragile when we think about merging migrations from 10 branches developed at the same time.

samdark commented 8 years ago

↑ is about parent-child relationships i.e. in the case all 10 new migrations will specify the same migration as the parent since it's the only one available at the moment these are created. Then there will be a problem about which migration should be executed first.

Faryshta commented 8 years ago

@samdark what do you mean? like circular dependencies? Or when two migrations depend on a parent migration?

If I need migration A to be executed before migration B then I just need to add A to the $depends property in migration B

class B extends Migration
{
    public $depends= [A::class];
}

The same as fixtures and assets.

samdark commented 8 years ago

The situation itself is OK. Logically dependency is correct but execution should be predictable and repeatable therefore we should define which order to use for migrations with equal dependencies.

Faryshta commented 8 years ago

@samdark how about having an internal constant?

class B extends Migration
{
     const CREATION_TIME = 1234567890; // unix timestamp
}

just first idea that came to mind.

samdark commented 8 years ago

We can use filemtime instead.

rob006 commented 8 years ago

Why change migrations names conventions in first place? If I have 20 migrations in one directory, it is better if they are sorted chronologically.

Faryshta commented 8 years ago

@rob006 mostly to be able to autoload them. As they are right now its impossible.

@samdark how do you propose it?

rob006 commented 8 years ago

As they are right now its impossible.

Why? Using namespace like app\migrations\m150416_155549_create_user_and_auth_tables will not work?

nkostadinov commented 8 years ago

One step at a time. I think the first thing is to add namespaces to migrations and make them PSR compliant. It will again require to specify migrationPath but when downgrading you wont have to and the classes will look more consistent.

samdark commented 8 years ago

@rob006 it's against PSR-2 which isn't really a good thing.

rob006 commented 8 years ago

app\migrations\M150416T155549CreateUserAndAuthTables?

samdark commented 8 years ago

Yes. That's better. Less human-readable but will be sorted right. That if we're talking about the old approach.

Overall the idea of @Faryshta is very interesting. At first I've denied it because it's different and I've got used to timestamp-based migrations but after more thinking it seems like a good approach. In fact migrations do depend on each other. You're starting new migration based on current database state so based on currently applied migration X. That solves some conflicting cases.

Faryshta commented 8 years ago

@samdark another issue is that sometimes you have to change your migrations order, if you add the timestamp on a constant/property/method then you can edit it later, on the filename its barely impossible without breaking something else, will work on a patch this weekend

samdark commented 8 years ago

Changing order after migrations got to repository sounds like a terrible idea to me.

Faryshta commented 8 years ago

lets say I made a table dinner_table and then I make a seemingly unrelated table waitress.

Then I get asked to associate each dinner_table to a waitress bu a waitress_id column.

On a developing stage is much easier to simply add the waitress_id column to the dinner_table migration and then run migrate/down, migrate/up than to make a new migration that depends on the previous ones.

its also cleaner when I have to check the migrations and run them for CI testing.

Again this is mostly for developing stage whent he customer usually changes the structure of the project at every meeting.

rob006 commented 8 years ago

You should create new migration that adds necessary changes and don't mess with migrations history. Your approach is unacceptable when you work with other people, because it creating problems that migration should solve.

Faryshta commented 8 years ago

Your approach is unacceptable when you work with other people, because it creating problems that migration should solve.

I send them an email 'syncronize migrations and run fixtures' and thats about the solution for my developing team. Yet again, this is for developing stages and much easier than "run yet another migration and see if the fixtures run"

rob006 commented 8 years ago

I send them an email 'syncronize migrations and run fixtures' and thats about the solution for my developing team.

This is exactly what I'm talking about. Migration are designed to update the database for the current state of project code. If you need another solution to synchronize database with current state of migrations you might as well not use them at all - use plain SQL dump or single migration with all db structure, and write migrations on release after development hell.

samdark commented 8 years ago

Here I agree with @rob006. If migration was pushed and it works, that's it. It's set in stone and should never be touched.

Faryshta commented 8 years ago

use plain SQL dump or single migration with all db structure, and write migrations on release after development hell.

The problem with that is when you want to run fixtures and testing on your application specially when including CI.

dynasource commented 8 years ago

I understand the reasoning, but this is way too complex. We should think from 1 dimension, otherwise you will get ambiguity issues. Its either a dependency mechanism or a m000000_000000 mechanism, not both.

Faryshta commented 8 years ago

@dynasource then lets think on another name for a non-linear tool to load the database schema.

Plus literally everything else in yii2 behaves non-lineary. Fixtures also deal with database data, and they load dependencies using hte Fixture::$depends property.

dynasource commented 8 years ago

well, the thing is that the current mechanism is built around m000000_000000, which implies a chronology based on seconds. If you want to include other mechinisms, this will easily get out of hand.

To give you an analogy. In a perfect world, we would have a perfect version control system for databases. However, should Yii support such a advanced system? Should we create a GIT for DB's in Yii? We have to keep it simple. The more complex we make it, the more support we have to give.

samdark commented 8 years ago

The idea of dependencies for migrations isn't bad and actually makes sense but as @dynasource said, we can't have two concepts at the same time.

Faryshta commented 8 years ago

@dynasource i am not saying to replace the current mechanism, i understand its being used already. I am saying that we can build a second mechanism with another name (revisions, syncronizations, etc). We can use this issue to decide about naming and implementations.

nkostadinov commented 8 years ago

It will bring unnecessary complexity. It is already complicated enough with just a simple feature of namespaced migrations which dont work well with path based migrations. In order to add dependency to migration you need to know what the migration does beforehand. There will be some unresolvable dependency cases, circular dependencies etc . It will not only bring dependency to migrations but also to the modules which contain that migration. Something like db composer - hell :)

dynasource commented 8 years ago

@faryshta, we all agree that having flexibility in the causality is great. We also agree on our fear to invest too many hours on this subject, ending up with our own migration framework (like composer, npm, git). I can also imagine that there are already third party packages available. However, if you are ambitious, we are not withholding you to create a separate yii extension for it of course.

samdark commented 8 years ago

Later this extension could replace current migrations if it will prove being good. I think this way is preferred since this is pretty new concept which needs to be proven.

Faryshta commented 8 years ago

instead of 'migration' which name should be used? I was thinking 'revision'.

Faryshta commented 8 years ago

or DbAsset

dynasource commented 8 years ago

migration :)

Faryshta commented 8 years ago

wouldn't it cause confussion to have a timestamp migrations and asset migrations? i would prefer to call them differently, besides the asset migrations will use things such as namespaces to load dependencies.

dynasource commented 8 years ago

it would be even more confusing if your extension about migrations does not contain the word migration. Unless your extension will get so popular and well known that it becomes a de facto standard. In that case you can call it anything you like ;)

Assets would not be preferable because its relates to files. Revisions relate to texts & articles, but is already better (covering a moment in time).

samdark commented 8 years ago

Calling these migrations is OK. All this stuff reminds me of how git works. If you haven't read Pro Git, I highly recommend it before thinking about architecture.

dynasource commented 8 years ago

@samdark, this Pro Git book seems quite useful in general. Shouldnt we put this link somewhere in this document:

samdark commented 8 years ago

Won't hurt.

samdark commented 8 years ago

https://github.com/yiisoft/yii2/commit/5ab5b4046cda66cfe759b722c3c26ca27cd12633