New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to use change method in migration instead of up/down #593

Closed
radzserg opened this Issue Jul 2, 2013 · 10 comments

Comments

Projects
None yet
6 participants
@radzserg

radzserg commented Jul 2, 2013

not a brand new idea. Of merging functionality of up and down functions in one change. Ror and https://github.com/robmorgan/phinx already have this feature.

i.e. if we have $this->createTable, addIndex etc we know that we need to delete table/column/index in down action.
The main problem here is tracking down the data that delete something that already exists. For example in up action we have dropTable dropColumn. In this case we don't know how to recreate table or column without additional actions.

That good thing that in most cases we will create rather than remove something. And this ability will save some time for developer.

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jul 2, 2013

My idea is following - rename current /yii/db/Migration to /yii/db/MigrationBase and create new Migration child of MigrationBase that will add additional functionality in functions that could support change behavior.

 public function addColumn($table, $column, $type)
    {
        if (!$this->_change || $this->_change == self::DIRECTION_UP) {
            return parent::addColumn($table, $column, $type);
        } else {
            $this->_changeDownQueue[] = function() use($table, $column) {
                    return parent::dropColumn($table, $column);
                };
        }
    }

after down migration just execute callbacks from queue in reverse order.

Well I got code for pull requests so if you approve this issue I'll pull ready solution.

radzserg commented Jul 2, 2013

My idea is following - rename current /yii/db/Migration to /yii/db/MigrationBase and create new Migration child of MigrationBase that will add additional functionality in functions that could support change behavior.

 public function addColumn($table, $column, $type)
    {
        if (!$this->_change || $this->_change == self::DIRECTION_UP) {
            return parent::addColumn($table, $column, $type);
        } else {
            $this->_changeDownQueue[] = function() use($table, $column) {
                    return parent::dropColumn($table, $column);
                };
        }
    }

after down migration just execute callbacks from queue in reverse order.

Well I got code for pull requests so if you approve this issue I'll pull ready solution.

@egorio

This comment has been minimized.

Show comment
Hide comment
@egorio

egorio Jul 2, 2013

Contributor

It looks useful but has some magic :)

Contributor

egorio commented Jul 2, 2013

It looks useful but has some magic :)

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jul 2, 2013

Well maybe... but this will be inside framework. For end developer this will looks like this

class m130702_094641_create_new_user extends yii\db\Migration
{
    public function change()
    {
        $this->createTable('user', [
            'id' => 'pk',
            'name' => 'text',
        ]);

        $this->createTable('order', [
            'id' => 'pk',
            'user_id' => 'int NOT NULL',
            'name' => 'text',
        ]);

        $this->addForeignKey('order_to_user', 'order', 'user_id', 'user', 'id');
    }
}

much less code

radzserg commented Jul 2, 2013

Well maybe... but this will be inside framework. For end developer this will looks like this

class m130702_094641_create_new_user extends yii\db\Migration
{
    public function change()
    {
        $this->createTable('user', [
            'id' => 'pk',
            'name' => 'text',
        ]);

        $this->createTable('order', [
            'id' => 'pk',
            'user_id' => 'int NOT NULL',
            'name' => 'text',
        ]);

        $this->addForeignKey('order_to_user', 'order', 'user_id', 'user', 'id');
    }
}

much less code

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jul 2, 2013

Member

@yiisoft/core-developers What do you think about this proposal?

Member

qiangxue commented Jul 2, 2013

@yiisoft/core-developers What do you think about this proposal?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jul 2, 2013

Member

Personally I don't like it because I think migrations should be with as less magic as possible and I'd never use it.

Member

samdark commented Jul 2, 2013

Personally I don't like it because I think migrations should be with as less magic as possible and I'd never use it.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jul 2, 2013

Member

May be better suited for an extension. Also too much magic in my opinion. FW should be simple, extension may add magic.

Member

cebe commented Jul 2, 2013

May be better suited for an extension. Also too much magic in my opinion. FW should be simple, extension may add magic.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jul 2, 2013

Member

Such behavior of course could save some time and efforts for the developer, but can have some side effects in the long perspective.

Actually it does not save much code. Here is common migration example:

public function up()
{
    $columns = array(
        ‘id’ => ‘pk’,
        ‘name’ => ‘string’,
    );
    $this->createTable(‘some_table’, $columns);
    $this->createIndex(‘idx_some_table _name’, ‘some_table’, ‘name’);
}
public function down()
{
    $this->dropTable(‘some_table’); // We are fighting for ability to remove this single line
}

Still this feature is helpful in case you perform a lot of “create” operations on the “up”:

public function up()
{
    $columns = array(
        …
    );
    $this->createTable(‘some_table_1’, $columns);
    $columns = array(
        …
    );
    $this->createTable(‘some_table_2’, $columns);
    $columns = array(
        …
    );
    $this->createTable(‘some_table_3’, $columns);
    $columns = array(
        …
    );
    $this->createTable(‘some_table_4’, $columns);
}
public function down()
{
    $this->dropTable(‘some_table_1’); 
    $this->dropTable(‘some_table_2’);
    $this->dropTable(‘some_table_3’);
    // It is easy to forget to drop something here.
}

The main problem with this feature is it can not be universal. While automatic reversion for the “create” operations is easy, for the “drop” operation it is impossible. If you drop some table at “up”, the migration can not automatically generate code to revert this at “down”.
After such feature is introduced, developers will stick to it and may become helpless in the uncommon situation. It is easy to forget why and how to write “down” method, if you do not use for a long time.

My opinion the migration should be left as it is now.

Member

klimov-paul commented Jul 2, 2013

Such behavior of course could save some time and efforts for the developer, but can have some side effects in the long perspective.

Actually it does not save much code. Here is common migration example:

public function up()
{
    $columns = array(
        ‘id’ => ‘pk’,
        ‘name’ => ‘string’,
    );
    $this->createTable(‘some_table’, $columns);
    $this->createIndex(‘idx_some_table _name’, ‘some_table’, ‘name’);
}
public function down()
{
    $this->dropTable(‘some_table’); // We are fighting for ability to remove this single line
}

Still this feature is helpful in case you perform a lot of “create” operations on the “up”:

public function up()
{
    $columns = array(
        …
    );
    $this->createTable(‘some_table_1’, $columns);
    $columns = array(
        …
    );
    $this->createTable(‘some_table_2’, $columns);
    $columns = array(
        …
    );
    $this->createTable(‘some_table_3’, $columns);
    $columns = array(
        …
    );
    $this->createTable(‘some_table_4’, $columns);
}
public function down()
{
    $this->dropTable(‘some_table_1’); 
    $this->dropTable(‘some_table_2’);
    $this->dropTable(‘some_table_3’);
    // It is easy to forget to drop something here.
}

The main problem with this feature is it can not be universal. While automatic reversion for the “create” operations is easy, for the “drop” operation it is impossible. If you drop some table at “up”, the migration can not automatically generate code to revert this at “down”.
After such feature is introduced, developers will stick to it and may become helpless in the uncommon situation. It is easy to forget why and how to write “down” method, if you do not use for a long time.

My opinion the migration should be left as it is now.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jul 2, 2013

Member

I have the same concerns as you all said. Closed.

Member

qiangxue commented Jul 2, 2013

I have the same concerns as you all said. Closed.

@qiangxue qiangxue closed this Jul 2, 2013

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jul 3, 2013

Well actually it should be addition to up/down not a replacement. This is what I wanted to propose https://gist.github.com/radzserg/5915803.

The example of @klimov-paul is quite tricky cause you change addForeignKey to createIndex. In this case - yes you only need to delete the table. In real example you need to delete foreign key and take care of operations order. i.e. delete foreign key then delete table.

radzserg commented Jul 3, 2013

Well actually it should be addition to up/down not a replacement. This is what I wanted to propose https://gist.github.com/radzserg/5915803.

The example of @klimov-paul is quite tricky cause you change addForeignKey to createIndex. In this case - yes you only need to delete the table. In real example you need to delete foreign key and take care of operations order. i.e. delete foreign key then delete table.

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jul 3, 2013

As for logic - it wlll be complicated only inside framework. As for user part - user will create absolutely the same migration but without down action, i.e. it will be simpler.

Well anyway it's your decision - I can't argue with you guys. Perhaps will leave for some extension.

radzserg commented Jul 3, 2013

As for logic - it wlll be complicated only inside framework. As for user part - user will create absolutely the same migration but without down action, i.e. it will be simpler.

Well anyway it's your decision - I can't argue with you guys. Perhaps will leave for some extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment