Fix removing autoincrement column from a primary key #786
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-1132 We use Jira to track the state of pull requests and the versions they got |
| @@ -654,6 +654,24 @@ protected function getPreAlterTableIndexForeignKeySQL(TableDiff $diff) | |||
| } | |||
| } | |||
|
|
|||
| // handle columns that are removed from changed indexes | |||
| foreach ($diff->changedIndexes as $chgKey => $chgIndex) { | |||
There was a problem hiding this comment.
Can this block be moved to a private method?
There was a problem hiding this comment.
changedIndexes should imho be handled on the same level as removedIndexes
There was a problem hiding this comment.
@andig it's just a question of readability
|
@deeky666 I think this change is fine. |
| $column = $diff->fromTable->getColumn($columnName); | ||
|
|
||
| if ($column->getAutoincrement() === true && in_array($columnName, $chgIndex->getColumns()) === false) { | ||
| $column->setAutoincrement(false); |
There was a problem hiding this comment.
I'm not sure if it's a good idea mutating the "from" table here as this piece of information might be needed later on by other parts of the table alteration (even if it was not the case at the moment). I'd better go a safe way here. So my suggestion is to either set it to true again after the SQL generation part or clone the column.
@Ocramius thoughts?
There was a problem hiding this comment.
@deeky666 not sure why- the same is being done for the removedIndexes https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php#L621?
There was a problem hiding this comment.
@andig I know that it is also done for removedIndexes and it might be causing the same issues there. So it was done wrong before and should not be done wrong again ;)
|
Besides my nitpicking it is looking good! |
| foreach ($diff->fromTable->getIndex($chgIndex->getName())->getColumns() as $columnName) { | ||
| $column = $diff->fromTable->getColumn($columnName); | ||
|
|
||
| if ($column->getAutoincrement() === true && in_array($columnName, $chgIndex->getColumns()) === false) { |
There was a problem hiding this comment.
You could add Index::hasColumn() but that is not absolutely required. I could also live with in_array() here ;)
No problem at all. If you're fine with the changes I'm happy to provide a squashed commit. |
|
btw, do you want a reference to DBAL-1132, too? |
|
as this seems pretty complete- any chance to get it committed? |
Fix removing autoincrement column from a primary key
|
Thanks @andig ! Merged manually (extracted a private method in the platform and fixed some minor glitches). |
#430 is only a partial fix for DBAL-464. This adds treating autoincrement columns that get removed when altering a primary key.