Drop type comments entirely

[greg0ire]

This feature is only useful for reverse-engineering purposes since we
have platform-aware comparison. Reverse-engineering is not enough to
justify having that feature, and ORM metadata reverse-engineered from
the database can still be adjusted afterwards.

[greg0ire]

🤔 I'm a bit surprised by the test suite result @morozov … it says there is a diff for a guid column on MariaDB, but after #4746 , that should not be the case anymore, right?

[greg0ire]

Debugged it, and it's just because of the comment of course :)
I think the one who has it is the $offlineTable

[greg0ire]

The remaining failures are with IBM DB2 and SQL Server, I don't know how to debug them, so I'm going to create the ORM and migration PRs instead

[morozov]

🤔 I'm a bit surprised by the test suite result … it says there is a diff for a guid column on MariaDB, but after #4746, that should not be the case anymore, right?

Which test are you referring to?

[morozov]

The test on SQL Server fails because of this:

dbal/src/Platforms/SQLServerPlatform.php

Lines 510 to 513 in d78267c

	$fromComment = $this->getColumnComment($columnDiff->fromColumn);
	$hasFromComment = ! empty($fromComment) || is_numeric($fromComment);
	if ($hasFromComment && $hasComment && $fromComment !== $comment) {

You replaced the call to $this->getColumnComment($column) with $column->getComment() in AbstractPlatform but not here. So when processing the diff, the platform thinks that the deployed column has a comment, tries to update it and fails because it doesn't exist:

EXEC sp_updateextendedproperty N'MS_Description', N'foo', N'SCHEMA', 'dbo', N'TABLE', 'sqlsrv_column_comment', N'COLUMN', commented_type
-- SQLSTATE [42000, 15217]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Property cannot be updated or deleted. Property 'MS_Description' does not exist for 'dbo.sqlsrv_column_comment.commented_type'.

The tests on IBM DB2 fail because ... surprise ... it doesn't support boolean columns, as well as most of the other platforms MySQL and Oracle. But the same test doesn't fail say on MySQL because of this hack:

dbal/src/Platforms/AbstractMySQLPlatform.php

Line 1052 in dfdb5fb

'tinyint' => 'boolean',

After some more research, of the two ComparatorTest::testDefaultValueComparison() failures on IBM DB2, only one is expected because even if the type is introspected incorrectly, the resulting SQL should be the same but it's slightly different:

- SMALLINT DEFAULT '0' NOT NULL
+ SMALLINT DEFAULT 0 NOT NULL

I would consider it a separate issue (#5108). Likely, similar to MariaDB, IBM DB2 provides an SQL expression representing the default values (the string 0 representing a numeric literal) while the DBAL considers it to be the value (i.e. a string). This is where the quotes are added:

dbal/src/Platforms/AbstractPlatform.php

Lines 2599 to 2601 in a4da59d

	if ($type instanceof Types\BooleanType) {
	return " DEFAULT '" . $this->convertBooleans($default) . "'";
	}

[greg0ire]

Which test are you referring to?

Sorry, I was referring to

dbal/tests/Functional/Schema/MySQLSchemaManagerTest.php

Lines 358 to 371 in 4d66064

	public function testDiffListGuidTableColumn(): void
	{
	$offlineTable = new Table('list_guid_table_column');
	$offlineTable->addColumn('col_guid', 'guid');
	$this->dropAndCreateTable($offlineTable);
	$onlineTable = $this->schemaManager->listTableDetails('list_guid_table_column');
	self::assertFalse(
	$this->schemaManager->createComparator()->diffTable($onlineTable, $offlineTable),
	'No differences should be detected with the offline vs online schema.'
	);
	}

[mvorisek]

No! Please do not remove DC2Type comments!

Following post #5194 (comment) .

a) Different DBAL Types, even if stored in the same DB column type, can have the values encoded differently. Without DC2Type comments, it is impossible to distinguish between such data/DB state and diff/migrate them correctly.

b) We use the DC2Type comments also in DB triggers to further enforce the data integrity. Without DC2Type comments, it is impossible to distinguish between different DBAL Types on the server side.

In summary, the DC2Type comments provide a very valuable and irreplaceable metadata stored together with the column definition, and they should stay forever.

[morozov]

The DBAL maintains column comments for its internal purposes which is irrelevant as of #4746.

If the application needs to maintain column comments for its own purposes, it should do so by its own means.

[greg0ire]

@morozov I'm not sure how to interpret the remaining failures… diffTable returns a non-empty diff, and that makes sense since the detected type is no longer the same as the expected one. Later on, when using toSql() on the schema diff that would know about this table diff, the difference should vanish, right? Should Doctrine\DBAL\Tests\Functional\Schema\ComparatorTest just be removed? Or should it maybe be rewritten to ensure that indeed, SchemaDiff::toSql() is empty?

[morozov]

diffTable returns a non-empty diff, and that makes sense since the detected type is no longer the same as the expected one.

It does and it doesn't. This is a remainder of the issue described in #5107 (comment) and partially fixed in #5108.

DB2 doesn't support boolean columns natively, so the DBAL emulates them via SMALLINT. The same applies to MySQL and Oracle. During schema introspection, the DBAL converts non-boolean columns back to boolean to satisfy the comparator:

1. dbal/src/Platforms/AbstractMySQLPlatform.php

   Line 1052 in dfdb5fb

   'tinyint' => 'boolean',

2. dbal/src/Schema/OracleSchemaManager.php

   Lines 146 to 147 in de2b2ef

   	} elseif ($precision === 1 && $scale === 0) {
   	$type = 'boolean';

But there's no hack for this for DB2, hence the failure. Most likely, we can reproduce a scenario where an integer column is mistakenly introspected as a boolean on these platforms and consider it a bug.

We could add the same hack for DB2 to satisfy the test but I'd argue that this condition in the comparator is the problem:

dbal/src/Platforms/AbstractPlatform.php

Line 4021 in de2b2ef

return $column1->getType() === $column2->getType();

In the context of schema migration, if two column declarations yield the same SQL, it doesn't matter if they are of the same type or not.

We could try removing this block and see if it yields any unexpected effects. Not having this condition may help get rid of the above hacks in the MySQL and Oracle platforms as well.

P.S.: doing the above would fix one of the two failures. The remaining one (where the platform unaware comparison is used) is a known limitation of the old API.

P.P.S.: given that the old comparison logic relies on the comments and it's still supported in 3.x, we can only drop the comments in 4.0 where the old API is not supported.

[greg0ire]

I'll extract the comparison loosening in a separate PR then.

[morozov]

Looks good overall, just needs some more cleanup. I believe the commits should be squashed. The first one will likely break the build.

[morozov]

Looks good!

I thought a bit about the upgrade impact, and the consequences look quite straightforward. If I have a commented type in 3.x which appends its type comment to the deployed column, when I upgrade to 4.0.0, there will be a schema diff since the DBAL will no longer expect the type comment and will remove it.

Maybe the above combined with some actual schema changes will produce unexpected results but I can't think of anything right now.

[mvorisek]

With this change, what method should be overrided to add a comment about the DBAL Type to DB column?

[greg0ire]

@mvorisek I don't know that there is one.

[bohanyang]

@morozov Just a question:

I have defined a custom type, which uses DC2Type comment as of now (3.5) .

After this change, will a new migration being generated (after diffing the schema) ?
even if no schema is changed
due to Doctrine cannot correctly detect a column's type
since it's custom defined and there's no DC2Type comment.

If yes, do you have any idea to solve this in the application?

Thanks!

PS: Read #4749

This way, the diff will be only generated if the column SQL representations on a given platform are different.

The idea seems legit. Just a little worried about how does it really work