Fix recreating indices for renamed/removed columns. #2134
Conversation
ndm2
force-pushed
the
0.x-fix-recreating-indices-for-renamed-removed-columns
branch
2 times, most recently
from
6e859b0 to
1c9f9e9
Compare
| return $this->copyAndDropTmpTable($instructions, $tableName); | ||
| $instructions = $this->bufferIndicesAndTriggers($instructions, $tableName); | ||
| $instructions = $this->updateIndicesForRenamedColumn($instructions, $columnName, $newColumnName); | ||
| $instructions = $this->copyAndDropTmpTable($instructions, $tableName); |
There was a problem hiding this comment.
Thoughts on modifying copyAndDropTmpTable to accept an optional third and fourth arguments $columnName and newColumnName, and then copyAndDropTmpTable is responsible for calling bufferIndicesAndTriggers, updateIndicesForRenamedColumn, and recreateIndicesAndTriggers?
This would cut down on the boilerplate around using these as three of them are always used in conjunction.
There was a problem hiding this comment.
I'm very much open to some refactoring here, I don't really like all that repitition either. I actually had it like this at first, with the new methods added around the copy/delete post step:
copyAndDropTmpTable(AlterInstructions $instructions, string $tableName, string $deletedOrRenamedColumnName, string $newColumnName)
It was kinda bloated, and I thought that it wouldn't be overly appreciated.
How about a new method that combines all the post step addition methods, something like endAlterByCopyTable to complement beginAlterByCopyTable?
There was a problem hiding this comment.
Sure, I'd be open to having a complimentary endAlterByCopyTable that wraps these 4 function calls as-is, makes sense as we have a beginAlterByCopyTable.
ndm2
force-pushed
the
0.x-fix-recreating-indices-for-renamed-removed-columns
branch
2 times, most recently
from
382f864 to
a4c6476
Compare
|
@MasterOdin Can we move forward here? You still had it on request change |
|
As a heads up, ideally #2131 is merged first, it will make rebasing less awkward, as there needs to be some structural changes here in case the other PRs foreign key check behavior is being kept. |
|
ping @MasterOdin |
|
Just realized I've linked the wrong PR, I didn't mean to say that this one should to be merged first, but the table drop one, sorry 😬 |
ndm2
force-pushed
the
0.x-fix-recreating-indices-for-renamed-removed-columns
branch
from
a4c6476 to
2557451
Compare
ndm2
force-pushed
the
0.x-fix-recreating-indices-for-renamed-removed-columns
branch
from
2557451 to
f6d45a8
Compare
A possible implementation to solve #2129, it ignores indices with expressions (which results in SQL errors accordingly when columns are missing on index recreation), and behaves like Postgres on column removal, ie it always removes the index, even composite ones.
Short of actually parsing the SQL, that regex for renaming is the best I could come up with so far. It can handle quite some variations, but I'm certainly not claiming that there aren't possibly cases where it would fail.