Schema Diff API cleanup #5622
Schema Diff API cleanup #5622
Conversation
Schema diff objects are meant to be instantiated only by schema comparators. As a cleanup of the old property-based schema comparison logic, we will eventually remove the $changedProperties parameter of the ColumnDiff class. Additionally, the following commit will mark ColumnDiff::$oldColumnName as deprecated rendering the corresponding constructor parameter obsolete.
morozov
force-pushed
the
schema-diff-cleanup
branch
from
ea3dfad to
fa5c72c
Compare
| . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnArray); | ||
|
|
||
| if ($columnDiff->fromColumn !== null) { | ||
| $oldColumn = $columnDiff->fromColumn; |
There was a problem hiding this comment.
I think it would be best to use oldColumn and oldColumnName as little as possible, and switch to fromColumn / fromColumnName as soon as possible (to minimize the diff between branches)
There was a problem hiding this comment.
I wouldn't say that the from/to naming is better than old/new (this is what GNU diff uses, for example). What diff between the branches are you referring to?
There was a problem hiding this comment.
I wouldn't say that the from/to naming is better than old/new (this is what GNU diff uses, for example).
I'm not saying it's better, but since we are using from/to as well, it might make sense to use the same naming everywhere.
What diff between the branches are you referring to?
The diff you would obtain when doing that kind of cleanup (switching to new naming) after merging up, I assumed that might have been your plan.
There was a problem hiding this comment.
So if I understand it correctly, it's about the variable names being used in the code, not any sort of API? Please see the updated version of the patch.
The $oldColumnName property and the corresponding constructor parameter were relevant until doctrine#220 (2.4.0) which introduced the $fromColumn property and the corresponding constructor parameter. Now they are redundant. The same applies to getOldColumnName(). The $fromColumn property contains all the properties of the old column including the name.
morozov
force-pushed
the
schema-diff-cleanup
branch
from
fa5c72c to
13efdc0
Compare
comparators. As a cleanup of the old property-based schema comparison logic, we will eventually remove the
$changedPropertiesparameter of theColumnDiffclass.$oldColumnNameproperty and the corresponding constructor parameter were relevant until Added support for alter table, foreign keys and autoincrement detection to Sqlite platform and schema #220 (2.4.0) which introduced the$fromColumnproperty and the corresponding constructor parameter. Now they are redundant. The same applies togetOldColumnName(). The$fromColumnproperty contains all the properties of the old column including the name.