Remove property-based schema comparison APIs #5649

morozov · 2022-09-03T06:07:14Z

TODO:

Deprecate and mark internal APIs related to schema comparison #5650

morozov · 2022-09-03T06:14:48Z

tests/Platforms/AbstractPlatformTestCase.php

-        $tableDiff->changedColumns['bar'] = new ColumnDiff(
-            new Column(
-                'baz',


This part of the test is invalid.

First of all, the column in $changedColumns cannot have its name changed. The old and the new column should have the same name to be added to $changedColumns:

dbal/src/Schema/Comparator.php

Lines 205 to 225 in 58d4b3c

foreach ($fromTableColumns as $column) {

$columnName = strtolower($column->getName());

// See if column is removed in "to" table.

if (! $toTable->hasColumn($columnName)) {

$tableDifferences->removedColumns[$columnName] = $column;

$changes++;

continue;

}

$toColumn = $toTable->getColumn($columnName);

if ($this->columnsEqual($column, $toColumn)) {

continue;

}

$tableDifferences->changedColumns[$column->getName()] = new ColumnDiff(

$toColumn,

$this->diffColumn($column, $toColumn),

$column,

);

Second, the test expects a rename column SQL to be generated for MySQL:

dbal/tests/Platforms/AbstractMySQLPlatformTestCase.php

Line 64 in 08630c6

. "CHANGE bar baz VARCHAR(255) DEFAULT 'def' NOT NULL, "

But not on Oracle:

dbal/tests/Platforms/OraclePlatformTest.php

Line 53 in 4bb2c0b

"ALTER TABLE mytable MODIFY (baz VARCHAR2(255) DEFAULT 'def' NOT NULL, "

Note that the latter assumes that the column name is already "baz", not "bar".

morozov · 2022-09-03T06:23:48Z

tests/Platforms/SQLServerPlatformTestCase.php

-        return [
-            'ALTER TABLE mytable ADD quota INT',
-            'ALTER TABLE mytable DROP COLUMN foo',
-            'ALTER TABLE mytable ALTER COLUMN baz NVARCHAR(255) NOT NULL',
-            "ALTER TABLE mytable ADD CONSTRAINT DF_6B2BD609_78240498 DEFAULT 'def' FOR baz",
-            'ALTER TABLE mytable ALTER COLUMN bloo BIT NOT NULL',
-            'ALTER TABLE mytable ADD CONSTRAINT DF_6B2BD609_CECED971 DEFAULT 0 FOR bloo',
-            "sp_rename 'mytable', 'userlist'",
-            "DECLARE @sql NVARCHAR(MAX) = N''; " .
-            "SELECT @sql += N'EXEC sp_rename N''' + dc.name + ''', N''' " .
-            "+ REPLACE(dc.name, '6B2BD609', 'E2B58069') + ''', ''json'';' " .
-            'FROM sys.default_constraints dc ' .
-            'JOIN sys.tables tbl ON dc.parent_object_id = tbl.object_id ' .
-            "WHERE tbl.name = 'userlist';" .
-            'EXEC sp_executesql @sql',


Assertions where the expected SQL was copy-pasted from the debugger don't add any value. The only thing they prove is that the generated SQL will not accidentally change which complicates changing code (including fixing bugs and refactoring). As the above comment shows, the expected SQL is not always correct.

morozov · 2022-09-03T06:24:43Z

tests/Schema/ComparatorTest.php

-    public function testDiffDecimalWithNullPrecision(): void
-    {
-        $column = new Column('foo', Type::getType('decimal'));
-        $column->setPrecision(null);
-
-        $column2 = new Column('foo', Type::getType('decimal'));
-
-        self::assertEquals([], $this->comparator->diffColumn($column, $column2));
-    }


This test only tests that the default precision is null. It is pointless.

morozov · 2022-09-03T06:25:25Z

tests/Schema/ComparatorTest.php

-    public function testCompareGuidColumns(): void
-    {
-        $column1 = new Column('foo', Type::getType('guid'), ['comment' => 'GUID 1']);
-        $column2 = new Column(
-            'foo',
-            Type::getType('guid'),
-            ['notnull' => false, 'length' => 36, 'fixed' => true, 'default' => 'NEWID()', 'comment' => 'GUID 2.'],
-        );
-
-        self::assertEquals(['notnull', 'default', 'comment'], $this->comparator->diffColumn($column1, $column2));
-        self::assertEquals(['notnull', 'default', 'comment'], $this->comparator->diffColumn($column2, $column1));
-    }


The comparator no longer needs to be aware of platform-specific type implementation details. This is taken care of in the platform-aware schema comparison.

morozov · 2022-09-03T06:26:21Z

tests/Schema/ComparatorTest.php

@@ -1064,39 +1058,6 @@ public function assertSchemaSequenceChangeCount(
        self::assertCount($removeSequenceCount, $diff->removedSequences);
    }

-    public function testDiffColumnPlatformOptions(): void


Changes in platform options are no longer exposed via the column diff as they were never used anywhere except for this test.

derrabus · 2022-09-05T11:05:28Z

src/Schema/ColumnDiff.php

+
+        // Null values need to be checked additionally as they tell whether to create or drop a default value.
+        // null != 0, null != false, null != '' etc. This affects platform's table alteration SQL generation.
+        if (($newDefault === null) !== ($oldDefault === null)) {


Suggested change

if (($newDefault === null) !== ($oldDefault === null)) {

if (($newDefault === null) xor ($oldDefault === null)) {

Maybe it's a me-only problem but !== on booleans makes my head spin. 😓

I'm curious what it would look like if we wanted to check them for equality 🤔

Let's start an RFC to add the xnor operator to PHP. 😁

morozov commented Sep 3, 2022

View reviewed changes

morozov force-pushed the remove-changed-properties branch from 1697596 to 343526d Compare September 3, 2022 06:19

morozov commented Sep 3, 2022

View reviewed changes

morozov force-pushed the remove-changed-properties branch from 343526d to c08335c Compare September 3, 2022 06:31

morozov mentioned this pull request Sep 3, 2022

Deprecate and mark internal APIs related to schema comparison #5650

Merged

morozov changed the title ~~Remove ColumnDiff::$changedProperties~~ Remove property-based schema comparison APIs Sep 3, 2022

morozov added this to the 4.0.0 milestone Sep 3, 2022

morozov added BC Break Schema Comparison labels Sep 3, 2022

morozov force-pushed the remove-changed-properties branch from c08335c to b983192 Compare September 3, 2022 18:11

morozov marked this pull request as ready for review September 3, 2022 18:15

morozov requested a review from greg0ire September 3, 2022 18:26

morozov force-pushed the remove-changed-properties branch from b983192 to b9141e9 Compare September 3, 2022 20:00

derrabus reviewed Sep 5, 2022

View reviewed changes

Remove property-based schema comparison APIs

e7b326c

morozov force-pushed the remove-changed-properties branch from b9141e9 to e7b326c Compare September 5, 2022 13:50

derrabus approved these changes Sep 7, 2022

View reviewed changes

morozov merged commit 1f4ca72 into doctrine:4.0.x Sep 7, 2022

morozov deleted the remove-changed-properties branch September 7, 2022 13:32

github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove property-based schema comparison APIs #5649

Remove property-based schema comparison APIs #5649

morozov commented Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022

morozov Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022

derrabus Sep 5, 2022 •

edited

Loading

morozov Sep 5, 2022

derrabus Sep 7, 2022

	foreach ($fromTableColumns as $column) {
	$columnName = strtolower($column->getName());

	// See if column is removed in "to" table.
	if (! $toTable->hasColumn($columnName)) {
	$tableDifferences->removedColumns[$columnName] = $column;
	$changes++;
	continue;
	}

	$toColumn = $toTable->getColumn($columnName);

	if ($this->columnsEqual($column, $toColumn)) {
	continue;
	}

	$tableDifferences->changedColumns[$column->getName()] = new ColumnDiff(
	$toColumn,
	$this->diffColumn($column, $toColumn),
	$column,
	);

	if (($newDefault === null) !== ($oldDefault === null)) {
	if (($newDefault === null) xor ($oldDefault === null)) {

Remove property-based schema comparison APIs #5649

Remove property-based schema comparison APIs #5649

Conversation

morozov commented Sep 3, 2022 • edited Loading

morozov Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

morozov Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

morozov Sep 3, 2022

Choose a reason for hiding this comment

morozov Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

morozov Sep 3, 2022

Choose a reason for hiding this comment

derrabus Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

morozov Sep 5, 2022

Choose a reason for hiding this comment

derrabus Sep 7, 2022

Choose a reason for hiding this comment

morozov commented Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022 •

edited

Loading

morozov Sep 3, 2022 •

edited

Loading

derrabus Sep 5, 2022 •

edited

Loading