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

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

Closed
stof opened this issue Jun 7, 2023 · 7 comments
Closed

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

stof opened this issue Jun 7, 2023 · 7 comments

Comments

@stof
Copy link
Member

stof commented Jun 7, 2023

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 ?

@greg0ire
Copy link
Member

greg0ire commented Jun 7, 2023

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.

@stof
Copy link
Member Author

stof commented Jun 7, 2023

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.

@greg0ire
Copy link
Member

greg0ire commented Jun 7, 2023

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.

@stof
Copy link
Member Author

stof commented Jun 7, 2023

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

@greg0ire
Copy link
Member

greg0ire commented Jun 7, 2023

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

@derrabus
Copy link
Member

derrabus commented Jun 8, 2023

Sure, go for it. 🙂

derrabus pushed a commit that referenced this issue Sep 14, 2023
|      Q       |   A
|------------- | -----------
| Type         | feature
| Fixed issues | closes #6056

#### Summary

This allows to opt-in for a schema generated without type comments once
the project is migrating to the new schema tooling that does not need
them, allowing to have a clean DB schema without waiting for DBAL 4.0
and to decouple the schema migration from the DBAL upgrade itself.

I decided to name that field `disable*` so that the opt-in for the new
behavior is done by setting it to `true`
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants