Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent column flagging as diff just because type differs (when one type is parent to other) #4016

Conversation

jesparic
Copy link

@jesparic jesparic commented May 20, 2020

Q A
Type improvement
BC Break no
Fixed issues No direct issue but builds on change made for #3427

Summary

The ability to extend the dbal type system allows us to define our own Enum implementations. Often these extend from the StringType built-in type class. Everything works as it should except for one caveat - when generating migration diffs the Comparator::diffColumn() will detect that the two types are not identical and add the column to the generated migration diff every time. This leaves us with 2 main choices:

  1. Manually remove the diffs from the generated migration files each time (not ideal and prevents us from using the diff in CI tasks effectively)
  2. Add Type::requiresSQLCommentHint() == true method to our extension class

Although a perfectly reasonable solution, in some cases #2 is less than ideal (i.e., if we don't want to make modifications to the schema columns). For example, when porting a legacy app where the source of truth (and legacy migration system) is still the old app. In this case, we use the diff not to migrate the changes, but instead to make our schema continually match the legacy app while the migration work continues (i.e., the legacy app can and will still change).

To satisfy the above use case, I have presented this pull request. I have added a small modification to the Comparator::diffColumn() method which basically checks if the second check type is a subclass of the parent and that the parent is a basic string type. If this is the case, then the 'type' diff is not added; i.e., Only detected differences in options will then cause the schema diff to report change to the column

To summarise, I think implementing an enum as an extension of the string class is a very common case and I don't see a strong reason to enforce/require requiresSQLCommentHint() for the type in this case (Doctrine is perfectly able to detect and hydrate the type correctly at runtime)

Kind regards,
Richard

@jesparic jesparic force-pushed the make-comparator-ignore-type-diff-when-is-parent branch 3 times, most recently from 86ca46b to 8131619 Compare May 21, 2020 19:41
…ype is parent to other)

This allows us, for example, to define an enum type that extends the string built-in and not be forced to modify the schema with Type::requiresSQLCommentHint()
@jesparic jesparic force-pushed the make-comparator-ignore-type-diff-when-is-parent branch from 8131619 to 45e708b Compare May 21, 2020 19:57
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this feature being in 2.10. If this will be considered to be added to DBAL, I would suggest the next minor version.

@@ -508,6 +513,15 @@ public function diffColumn(Column $column1, Column $column2)
return array_unique($changedProperties);
}

private function isStringParentOf(string $type1, string $type2) : bool
{
if ($type1 !== Types\StringType::class) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often these extend from the StringType built-in type class.

This seems to be a hard coded specific change for your enum use case. What about types that use StringType too but are supposed to be mentioned in changes? I personally wouldn't build upon an existing type.

@jesparic jesparic closed this Jul 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants