dbal icon indicating copy to clipboard operation
dbal copied to clipboard

Add a setting to opt-in for skipping type comments in the schema

Open stof opened this issue 1 year ago • 6 comments

Feature Request

Q A
New Feature yes
RFC no

Summary

When using the platform-aware schema comparison, the type comments are useless (which is why they will be gone in 3.0).

For a project attempting to migrate from DateTimeTzType to DateTimeTzImmutableType (or any of the other type pairs based on DateTime vs DateTimeImmutable) in order to migrate to DateTimeImmutable, the schema tool would currently add type comments for all those date fields instead of not even having a database migration at all.

It would be great to have a configuration setting to opt-in for the new behavior (not generating type comments) already to avoid those database migrations. When turned on, \Doctrine\DBAL\Platforms\AbstractPlatform::getColumnComment would always skip adding the type comment (without even calling column->getType()->requiresSQLCommentHint($this)).

What do you think about that ?

stof avatar Jun 07 '23 12:06 stof

I think it's a good idea. It would still result in one massive migration, right? Anyway, what I like most about this is that it would expose potential issues we might have missed before we even release 4.0.0.

greg0ire avatar Jun 07 '23 13:06 greg0ire

No, it would result in no migration at all when migrating from the mutable types to the immutable types with this setting enabled as the database schema would be exactly the same (the only difference between those types will be the PHP conversion layer, which does not impact DB migrations).

Of course, if you are already using immutable types today, enabling this setting will generate a migration to remove those comments. But that is the same migration than the one you would have when you migrate from 3.6 to 4.0 (as 4.0 removes those comments). The difference is that you could opt-in for this migration separately from the 4.0 upgrade.

stof avatar Jun 07 '23 13:06 stof

We wouldn't do this solely for immutable types though, would we? That's what I'm referring to with "massive migration": it would impact all columns with a DC2Type comment on it, right? Nothing negative about this BTW, I just want to make sure I'm understanding this fully.

The difference is that you could opt-in for this migration separately from the 4.0 upgrade.

That is a really great reason to go forward with what you are proposing.

greg0ire avatar Jun 07 '23 13:06 greg0ire

Indeed, if you use one of the other types that is currently marked as being commented, the comment would also be removed there.

Here is the list of types that use it in the core:

  • all the *_immutable variants of date-related types
  • guid type for platforms that don't have a native type for UUID (but would you use it if your platform does not have a native type as the common use case is to use it for ids, for which you want efficient indexing rather than text-based indexing ?)
  • json type for platforms without native type for json
  • boolean type for the DB2 platform
  • array and object types, which are deprecated

stof avatar Jun 07 '23 15:06 stof

@derrabus @morozov , I think that would be great. Thoughts?

greg0ire avatar Jun 07 '23 18:06 greg0ire

Sure, go for it. 🙂

derrabus avatar Jun 08 '23 11:06 derrabus