Remove column platform option getters#6975
Merged
morozov merged 2 commits intodoctrine:5.0.xfrom Jun 1, 2025
Merged
Conversation
greg0ire
approved these changes
Jun 1, 2025
Comment on lines
+131
to
+136
| [ | ||
| static fn (Column $column): ?string => $column->getCharset(), | ||
| static fn (Column $column): ?string => $column->getCollation(), | ||
| static fn (Column $column): mixed => $column->getMinimumValue(), | ||
| static fn (Column $column): mixed => $column->getMaximumValue(), | ||
| ] as $property |
Contributor
There was a problem hiding this comment.
Since more property exists in the PlatformOptions, like, at least, enumType this doesn't give the same behavior (cf #7238)
Before when changing the enumType this method returned true ; now it returns false. (and maybe there are others important platform options)
Not sure of the impact
| unset($options['collation']); | ||
| $column->setPlatformOptions($options); | ||
| ($editor ??= $table->edit()) | ||
| ->modifyColumn($column->getObjectName(), static function (ColumnEditor $editor): void { |
Contributor
There was a problem hiding this comment.
Currently
unset($options['collation']);
$column->setPlatformOptions($options);
and modifyColumn are not equivalent, because it loosing all the extra platform options not handled in the ColumnEditor, like enumType (cf #7238)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes the methods deprecated in #6945. We cannot remove the setters yet (they are not yet deprecated) because there is yet no way to modify a column, which is a part of a table, which is a part of a schema (we need a schema editor).
I'm also leaving
ColumnDiff::hasPlatformOptionsChanged()in place for now. I believe that replacing it with one more new method per property would be overkill. We can approach replacing it with something else separately.As a side note / observation, the logic of normalizing tables in comparators in order to account for some default charset/collation behavior, even though it is correct, might be inefficient. We construct a new graph of objects only in order to compare it with another one as is. From the CPU/memory usage perspective, it would be more efficient to actually make the comparison more intelligent w/o creating intermediate/normalized values.