Skip to content

Commit c0a0d37

Browse files
dereuromarkdereuromark
authored
Merge pull request #1618 from MasterOdin/column_after_fk
Fix regressions in migrations dealing with dropping columns/tables in same step as foreign keys and indexes
2 parents f5fb347 + 3397743 commit c0a0d37

File tree

7 files changed

+410
-8
lines changed

7 files changed

+410
-8
lines changed

‎src/Phinx/Db/Plan/Plan.php‎

Lines changed: 100 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,13 @@ class Plan
8787
*/
8888
protected $constraints = [];
8989

90+
/**
91+
* List of dropped columns
92+
*
93+
* @var \Phinx\Db\Plan\AlterTable[]
94+
*/
95+
protected $columnRemoves = [];
96+
9097
/**
9198
* Constructor
9299
*
@@ -124,7 +131,24 @@ protected function updatesSequence()
124131
$this->tableUpdates,
125132
$this->constraints,
126133
$this->indexes,
134+
$this->columnRemoves,
135+
$this->tableMoves,
136+
];
137+
}
138+
139+
/**
140+
* Returns a nested list of all the steps to execute in inverse order
141+
*
142+
* @return AlterTable[][]
143+
*/
144+
protected function inverseUpdatesSequence()
145+
{
146+
return [
147+
$this->constraints,
127148
$this->tableMoves,
149+
$this->indexes,
150+
$this->columnRemoves,
151+
$this->tableUpdates
128152
];
129153
}
130154

@@ -155,7 +179,7 @@ public function execute(AdapterInterface $executor)
155179
*/
156180
public function executeInverse(AdapterInterface $executor)
157181
{
158-
collection(array_reverse($this->updatesSequence()))
182+
collection($this->inverseUpdatesSequence())
159183
->unfold()
160184
->each(function ($updates) use ($executor) {
161185
$executor->executeActions($updates->getTable(), $updates->getActions());
@@ -183,6 +207,20 @@ protected function resolveConflicts()
183207
$this->tableUpdates = $this->forgetTable($action->getTable(), $this->tableUpdates);
184208
$this->constraints = $this->forgetTable($action->getTable(), $this->constraints);
185209
$this->indexes = $this->forgetTable($action->getTable(), $this->indexes);
210+
$this->columnRemoves = $this->forgetTable($action->getTable(), $this->columnRemoves);
211+
}
212+
}
213+
214+
// Columns that are dropped will automatically cause the indexes to be dropped as well
215+
foreach ($this->columnRemoves as $columnRemove) {
216+
foreach ($columnRemove->getActions() as $action) {
217+
if ($action instanceof RemoveColumn) {
218+
list($this->indexes) = $this->forgetDropIndex(
219+
$action->getTable(),
220+
[$action->getColumn()->getName()],
221+
$this->indexes
222+
);
223+
}
186224
}
187225
}
188226

@@ -198,7 +236,7 @@ protected function resolveConflicts()
198236
$this->constraints = collection($this->constraints)
199237
->map(function (AlterTable $alter) {
200238
// Dropping indexes used by foreign keys is a conflict, but one we can resolve
201-
// if the foreign key is also scheduled to be dropped. If we can find sucha a case,
239+
// if the foreign key is also scheduled to be dropped. If we can find such a a case,
202240
// we force the execution of the index drop after the foreign key is dropped.
203241
return $this->remapContraintAndIndexConflicts($alter);
204242
})
@@ -257,11 +295,12 @@ protected function remapContraintAndIndexConflicts(AlterTable $alter)
257295
$this->indexes
258296
);
259297

298+
$return = [$action];
260299
if (!empty($dropIndexActions)) {
261-
return array_merge([$action], $dropIndexActions);
300+
$return = array_merge($return, $dropIndexActions);
262301
}
263302

264-
return [$action];
303+
return $return;
265304
})
266305
->each(function ($action) use ($newAlter) {
267306
$newAlter->addAction($action);
@@ -317,6 +356,53 @@ protected function forgetDropIndex(Table $table, array $columns, array $actions)
317356
return [$indexes, $dropIndexActions->getArrayCopy()];
318357
}
319358

359+
/**
360+
* Deletes any RemoveColumn actions for the given table and exact columns
361+
*
362+
* @param Table $table The table to find in the list of actions
363+
* @param string[] $columns The column names to match
364+
* @param AlterTable[] $actions The actions to transform
365+
* @return array A tuple containing the list of actions without actions for removing the column
366+
* and a list of remove column actions that were removed.
367+
*/
368+
protected function forgetRemoveColumn(Table $table, array $columns, array $actions)
369+
{
370+
$removeColumnActions = new ArrayObject();
371+
$indexes = collection($actions)
372+
->map(function ($alter) use ($table, $columns, $removeColumnActions) {
373+
if ($alter->getTable()->getName() !== $table->getName()) {
374+
return $alter;
375+
}
376+
377+
$newAlter = new AlterTable($table);
378+
collection($alter->getActions())
379+
->map(function ($action) use ($columns) {
380+
if (!$action instanceof RemoveColumn) {
381+
return [$action, null];
382+
}
383+
if (in_array($action->getColumn(), $columns)) {
384+
return [null, $action];
385+
}
386+
387+
return [$action, null];
388+
})
389+
->each(function ($tuple) use ($newAlter, $removeColumnActions) {
390+
list($action, $removeColumn) = $tuple;
391+
if ($action) {
392+
$newAlter->addAction($action);
393+
}
394+
if ($removeColumn) {
395+
$removeColumnActions->append($removeColumn);
396+
}
397+
});
398+
399+
return $newAlter;
400+
})
401+
->toArray();
402+
403+
return [$indexes, $removeColumnActions->getArrayCopy()];
404+
}
405+
320406
/**
321407
* Collects all table creation actions from the given intent
322408
*
@@ -380,11 +466,17 @@ protected function gatherUpdates($actions)
380466
$table = $action->getTable();
381467
$name = $table->getName();
382468

383-
if (!isset($this->tableUpdates[$name])) {
384-
$this->tableUpdates[$name] = new AlterTable($table);
469+
if ($action instanceof RemoveColumn) {
470+
if (!isset($this->columnRemoves[$name])) {
471+
$this->columnRemoves[$name] = new AlterTable($table);
472+
}
473+
$this->columnRemoves[$name]->addAction($action);
474+
} else {
475+
if (!isset($this->tableUpdates[$name])) {
476+
$this->tableUpdates[$name] = new AlterTable($table);
477+
}
478+
$this->tableUpdates[$name]->addAction($action);
385479
}
386-
387-
$this->tableUpdates[$name]->addAction($action);
388480
});
389481
}
390482

‎tests/Phinx/Migration/ManagerTest.php‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5583,6 +5583,49 @@ public function testReversibleMigrationWithIndexConflict()
55835583
$this->assertFalse($adapter->hasIndex('my_table', ['entity_id']));
55845584
}
55855585

5586+
public function testReversibleMigrationWithFKConflictOnTableDrop()
5587+
{
5588+
if (!TESTS_PHINX_DB_ADAPTER_MYSQL_ENABLED) {
5589+
$this->markTestSkipped('Mysql tests disabled. See TESTS_PHINX_DB_ADAPTER_MYSQL_ENABLED constant.');
5590+
}
5591+
$configArray = $this->getConfigArray();
5592+
$adapter = $this->manager->getEnvironment('production')->getAdapter();
5593+
5594+
// override the migrations directory to use the reversible migrations
5595+
$configArray['paths']['migrations'] = $this->getCorrectedPath(__DIR__ . '/_files/drop_table_with_fk_regression');
5596+
$config = new Config($configArray);
5597+
5598+
// ensure the database is empty
5599+
$adapter->dropDatabase(TESTS_PHINX_DB_ADAPTER_MYSQL_DATABASE);
5600+
$adapter->createDatabase(TESTS_PHINX_DB_ADAPTER_MYSQL_DATABASE);
5601+
$adapter->disconnect();
5602+
5603+
// migrate to the latest version
5604+
$this->manager->setConfig($config);
5605+
$this->manager->migrate('production');
5606+
5607+
// ensure up migrations worked
5608+
$this->assertTrue($adapter->hasTable('orders'));
5609+
$this->assertTrue($adapter->hasTable('customers'));
5610+
$this->assertTrue($adapter->hasColumn('orders', 'order_date'));
5611+
$this->assertTrue($adapter->hasColumn('orders', 'customer_id'));
5612+
$this->assertTrue($adapter->hasForeignKey('orders', ['customer_id']));
5613+
5614+
// revert all changes to the first
5615+
$this->manager->rollback('production', '20190928205056');
5616+
5617+
// ensure reversed migrations worked
5618+
$this->assertTrue($adapter->hasTable('orders'));
5619+
$this->assertTrue($adapter->hasColumn('orders', 'order_date'));
5620+
$this->assertFalse($adapter->hasColumn('orders', 'customer_id'));
5621+
$this->assertFalse($adapter->hasTable('customers'));
5622+
$this->assertFalse($adapter->hasForeignKey('orders', ['customer_id']));
5623+
5624+
$this->manager->rollback('production');
5625+
$this->assertFalse($adapter->hasTable('orders'));
5626+
$this->assertFalse($adapter->hasTable('customers'));
5627+
}
5628+
55865629
public function testReversibleMigrationsWorkAsExpectedWithNamespace()
55875630
{
55885631
if (!TESTS_PHINX_DB_ADAPTER_MYSQL_ENABLED) {
@@ -5943,6 +5986,64 @@ public function testPostgresFullMigration()
59435986
$this->assertFalse($adapter->hasTable('users'));
59445987
}
59455988

5989+
public function testMigrationWithDropColumnAndForeignKeyAndIndex()
5990+
{
5991+
if (!TESTS_PHINX_DB_ADAPTER_MYSQL_ENABLED) {
5992+
$this->markTestSkipped('Mysql tests disabled. See TESTS_PHINX_DB_ADAPTER_MYSQL_ENABLED constant.');
5993+
}
5994+
$configArray = $this->getConfigArray();
5995+
$adapter = $this->manager->getEnvironment('production')->getAdapter();
5996+
5997+
// override the migrations directory to use the reversible migrations
5998+
$configArray['paths']['migrations'] = $this->getCorrectedPath(__DIR__ . '/_files/drop_column_fk_index_regression');
5999+
$config = new Config($configArray);
6000+
6001+
// ensure the database is empty
6002+
$adapter->dropDatabase(TESTS_PHINX_DB_ADAPTER_MYSQL_DATABASE);
6003+
$adapter->createDatabase(TESTS_PHINX_DB_ADAPTER_MYSQL_DATABASE);
6004+
$adapter->disconnect();
6005+
6006+
$this->manager->setConfig($config);
6007+
$this->manager->migrate('production', '20190928205056');
6008+
6009+
$this->assertTrue($adapter->hasTable('table1'));
6010+
$this->assertTrue($adapter->hasTable('table2'));
6011+
$this->assertTrue($adapter->hasTable('table3'));
6012+
$this->assertTrue($adapter->hasColumn('table1', 'table2_id'));
6013+
$this->assertTrue($adapter->hasForeignKey('table1', ['table2_id'], 'table1_table2_id'));
6014+
$this->assertTrue($adapter->hasIndexByName('table1', 'table1_table2_id'));
6015+
$this->assertTrue($adapter->hasColumn('table1', 'table3_id'));
6016+
$this->assertTrue($adapter->hasForeignKey('table1', ['table3_id'], 'table1_table3_id'));
6017+
$this->assertTrue($adapter->hasIndexByName('table1', 'table1_table3_id'));
6018+
6019+
// Run the next migration
6020+
$this->manager->migrate('production');
6021+
$this->assertTrue($adapter->hasTable('table1'));
6022+
$this->assertTrue($adapter->hasTable('table2'));
6023+
$this->assertTrue($adapter->hasTable('table3'));
6024+
$this->assertTrue($adapter->hasColumn('table1', 'table2_id'));
6025+
$this->assertTrue($adapter->hasForeignKey('table1', ['table2_id'], 'table1_table2_id'));
6026+
$this->assertTrue($adapter->hasIndexByName('table1', 'table1_table2_id'));
6027+
$this->assertFalse($adapter->hasColumn('table1', 'table3_id'));
6028+
$this->assertFalse($adapter->hasForeignKey('table1', ['table3_id'], 'table1_table3_id'));
6029+
$this->assertFalse($adapter->hasIndexByName('table1', 'table1_table3_id'));
6030+
6031+
// rollback
6032+
$this->manager->rollback('production');
6033+
$this->manager->rollback('production');
6034+
6035+
// ensure reversed migrations worked
6036+
$this->assertTrue($adapter->hasTable('table1'));
6037+
$this->assertTrue($adapter->hasTable('table2'));
6038+
$this->assertTrue($adapter->hasTable('table3'));
6039+
$this->assertFalse($adapter->hasColumn('table1', 'table2_id'));
6040+
$this->assertFalse($adapter->hasForeignKey('table1', ['table2_id'], 'table1_table2_id'));
6041+
$this->assertFalse($adapter->hasIndexByName('table1', 'table1_table2_id'));
6042+
$this->assertFalse($adapter->hasColumn('table1', 'table3_id'));
6043+
$this->assertFalse($adapter->hasForeignKey('table1', ['table3_id'], 'table1_table3_id'));
6044+
$this->assertFalse($adapter->hasIndexByName('table1', 'table1_table3_id'));
6045+
}
6046+
59466047
public function setExpectedException($exceptionName, $exceptionMessage = '', $exceptionCode = null)
59476048
{
59486049
if (method_exists($this, 'expectException')) {

‎tests/Phinx/Migration/_files/drop_column_fk_index_regression/20190928205056_first_fk_index_migration.php‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
<?php
2+
3+
use Phinx\Migration\AbstractMigration;
4+
5+
class FirstFkIndexMigration extends AbstractMigration
6+
{
7+
public function up()
8+
{
9+
$this->table('table2', [
10+
'id' => false,
11+
'primary_key' => ['id'],
12+
'engine' => 'InnoDB',
13+
'encoding' => 'utf8',
14+
'collation' => 'utf8_unicode_ci',
15+
'comment' => '',
16+
'row_format' => 'DYNAMIC',
17+
])
18+
->addColumn('id', 'integer', [
19+
'null' => false,
20+
'limit' => 20,
21+
'identity' => 'enable',
22+
])
23+
->create();
24+
25+
$this->table('table3', [
26+
'id' => false,
27+
'primary_key' => ['id'],
28+
'engine' => 'InnoDB',
29+
'encoding' => 'utf8',
30+
'collation' => 'utf8_unicode_ci',
31+
'comment' => '',
32+
'row_format' => 'DYNAMIC',
33+
])
34+
->addColumn('id', 'integer', [
35+
'null' => false,
36+
'limit' => 20,
37+
'identity' => 'enable',
38+
])
39+
->create();
40+
41+
$this->table('table1', [
42+
'id' => false,
43+
'primary_key' => ['id'],
44+
'engine' => 'InnoDB',
45+
'encoding' => 'utf8',
46+
'collation' => 'utf8_unicode_ci',
47+
'comment' => '',
48+
'row_format' => 'DYNAMIC',
49+
])
50+
->addColumn('id', 'integer', [
51+
'null' => false,
52+
'limit' => 20,
53+
'identity' => 'enable',
54+
])
55+
->addColumn('table2_id', 'integer', [
56+
'null' => true,
57+
'limit' => 20,
58+
'after' => 'id',
59+
])
60+
->addIndex(['table2_id'], [
61+
'name' => 'table1_table2_id',
62+
'unique' => false,
63+
])
64+
->addForeignKey('table2_id', 'table2', 'id', [
65+
'constraint' => 'table1_table2_id',
66+
'update' => 'RESTRICT',
67+
'delete' => 'RESTRICT',
68+
])
69+
->addColumn('table3_id', 'integer', [
70+
'null' => true,
71+
'limit' => 20,
72+
])
73+
->addIndex(['table3_id'], [
74+
'name' => 'table1_table3_id',
75+
'unique' => false,
76+
])
77+
->addForeignKey('table3_id', 'table3', 'id', [
78+
'constraint' => 'table1_table3_id',
79+
'update' => 'RESTRICT',
80+
'delete' => 'RESTRICT',
81+
])
82+
->create();
83+
}
84+
85+
public function down()
86+
{
87+
$this->table('table1', [
88+
'id' => false,
89+
'primary_key' => ['id'],
90+
'engine' => 'InnoDB',
91+
'encoding' => 'utf8',
92+
'collation' => 'utf8_unicode_ci',
93+
'comment' => '',
94+
'row_format' => 'DYNAMIC',
95+
])
96+
->removeColumn('table2_id')
97+
->removeIndexByName("table1_table2_id")
98+
->dropForeignKey('table2_id', 'table1_table2_id')
99+
->update();
100+
}
101+
}

0 commit comments

Comments
 (0)