Phinx: Rollback doesn't remove tables created via `change()`

[Stalinko]

Just wasted half-hour because of this bug. The title is quite self-explanatory.

Make some migration which creates a table:

<?php

use Phinx\Migration\AbstractMigration;

class CreateTestTable extends AbstractMigration
{
    public function change()
    {
        $this->table('test')
            ->addColumn('name', 'string')
            ->save();
    }
}

Then do migrate and rollback. At rollback it will simply delete all columns but id and keep the table.

[chinpei215]

Probably, you would need to use create() instead of save(). Since save() does either update() or create(), I think it is not easy to rollback to the original state.

[Stalinko]

@chinpei215, you're absolutely right! Thanks.
Probably it would be nice to state that case in the docs, because the docs suggest to create tables via save() not create() (see https://book.cakephp.org/3.0/en/phinx/migrations.html#creating-a-table)

Or even raise a warning when trying to rollback a migration with change() because it's not possible to determine whether the table must be removed or not.

[chinpei215]

@Stalinko You are welcome. And probably you are right. I think the document should be fixed.

[lorenzo]

Closing as this will not be a requirement anymore when the internals refactoring branch is merged

[chinpei215]

I cannot confirm that this issue has been fixed. Though I think this is documentation issue, if you were planing to fix this issue by internal changes, please confirm this again.

[lorenzo]

@chinpei215 You mean that calling create or update is required instead of just calling save? I was quite certain that I fixed in 0.10

[chinpei215]

@lorenzo I mean that people need to call create() when creating tables. If you call save() when creating tables like this example, those tables will be not removed after rolled back.

[lorenzo]

@chinpei215 is that still the case in 0.10?

[chinpei215]

@lorenzo Yes. If I run the above example shown in this issue, the test table is not removed after rolled back:

mysql> desc test;
+-------+--------------+------+-----+---------+----------------+
| Field | Type         | Null | Key | Default | Extra          |
+-------+--------------+------+-----+---------+----------------+
| id    | int(11)      | NO   | PRI | NULL    | auto_increment |
| name  | varchar(255) | NO   |     | NULL    |                |
+-------+--------------+------+-----+---------+----------------+
2 rows in set (0.00 sec)

mysql> desc test;
+-------+---------+------+-----+---------+----------------+
| Field | Type    | Null | Key | Default | Extra          |
+-------+---------+------+-----+---------+----------------+
| id    | int(11) | NO   | PRI | NULL    | auto_increment |
+-------+---------+------+-----+---------+----------------+
1 row in set (0.00 sec)

[lorenzo]

crap, I need to restore those warnings, then. I though the need for this was gone :(

[chinpei215]

I think it is difficult to fix this issue. In my opinion, if we want to do that, probably we would need to change phinxlog schema, and mark when tables were created. I am not sure if it is a good idea. So I think it is a documentation issue.
As another option, we could throw warnings when tables were created by save(). (or throw errors? it breaks BC though).