fix: TableDiff deprecation points to wrong method (#6273)

Follow up of #6080 While preparing the merge request to port to 4.0.x I found that some comments point to a deprecated method (one even points to itself) I also found that in DB2 the reorg happens even if there is no query running which was a bit weird and adding column was not triggering a reorg which is wrong and was not picked up by tests (because other changed columns triggered it)
doctrine · Jan 22, 2024 · 06d9e9f · 06d9e9f
1 parent 6375864
commit 06d9e9f
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 21 deletions.
diff --git a/src/Platforms/DB2Platform.php b/src/Platforms/DB2Platform.php
@@ -591,6 +591,8 @@ public function getAlterTableSQL(TableDiff $diff)
         $tableNameSQL = ($diff->getOldTable() ?? $diff->getName($this))->getQuotedName($this);
 
         $queryParts = [];
+        $needsReorg = false;
+
         foreach ($diff->getAddedColumns() as $column) {
             if ($this->onSchemaAlterTableAddColumn($column, $diff, $columnSql)) {
                 continue;
@@ -610,7 +612,8 @@ public function getAlterTableSQL(TableDiff $diff)
 
             $queryParts[] = $queryPart;
 
-            $comment = $this->getColumnComment($column);
+            $comment    = $this->getColumnComment($column);
+            $needsReorg = true;
 
             if ($comment === null || $comment === '') {
                 continue;
@@ -623,7 +626,6 @@ public function getAlterTableSQL(TableDiff $diff)
             );
         }
 
-        $needsReorg = false;
         foreach ($diff->getDroppedColumns() as $column) {
             if ($this->onSchemaAlterTableRemoveColumn($column, $diff, $columnSql)) {
                 continue;
@@ -660,13 +662,8 @@ public function getAlterTableSQL(TableDiff $diff)
                 $columnDiff,
                 $sql,
                 $queryParts,
+                $needsReorg,
             );
-
-            if (empty($columnDiff->changedProperties)) {
-                continue;
-            }
-
-            $needsReorg = true;
         }
 
         $tableSql = [];
@@ -732,11 +729,12 @@ private function gatherAlterColumnSQL(
         string $table,
         ColumnDiff $columnDiff,
         array &$sql,
-        array &$queryParts
+        array &$queryParts,
+        bool &$needsReorg
     ): void {
-        $alterColumnClauses = $this->getAlterColumnClausesSQL($columnDiff);
+        $alterColumnClauses = $this->getAlterColumnClausesSQL($columnDiff, $needsReorg);
 
-        if (empty($alterColumnClauses)) {
+        if (count($alterColumnClauses) < 1) {
             return;
         }
 
@@ -760,7 +758,7 @@ private function gatherAlterColumnSQL(
      *
      * @return string[]
      */
-    private function getAlterColumnClausesSQL(ColumnDiff $columnDiff): array
+    private function getAlterColumnClausesSQL(ColumnDiff $columnDiff, bool &$needsReorg): array
     {
         $newColumn = $columnDiff->getNewColumn()->toArray();
 
@@ -770,6 +768,8 @@ private function getAlterColumnClausesSQL(ColumnDiff $columnDiff): array
         $alterClause = 'ALTER COLUMN ' . $newName;
 
         if ($newColumn['columnDefinition'] !== null) {
+            $needsReorg = true;
+
             return [$alterClause . ' ' . $newColumn['columnDefinition']];
         }
 
@@ -786,22 +786,26 @@ private function getAlterColumnClausesSQL(ColumnDiff $columnDiff): array
             $columnDiff->hasScaleChanged() ||
             $columnDiff->hasFixedChanged()
         ) {
-            $clauses[] = $alterClause . ' SET DATA TYPE ' . $newColumn['type']->getSQLDeclaration($newColumn, $this);
+            $needsReorg = true;
+            $clauses[]  = $alterClause . ' SET DATA TYPE ' . $newColumn['type']->getSQLDeclaration($newColumn, $this);
         }
 
         if ($columnDiff->hasNotNullChanged()) {
-            $clauses[] = $newColumn['notnull'] ? $alterClause . ' SET NOT NULL' : $alterClause . ' DROP NOT NULL';
+            $needsReorg = true;
+            $clauses[]  = $newColumn['notnull'] ? $alterClause . ' SET NOT NULL' : $alterClause . ' DROP NOT NULL';
         }
 
         if ($columnDiff->hasDefaultChanged()) {
             if (isset($newColumn['default'])) {
                 $defaultClause = $this->getDefaultValueDeclarationSQL($newColumn);
 
                 if ($defaultClause !== '') {
-                    $clauses[] = $alterClause . ' SET' . $defaultClause;
+                    $needsReorg = true;
+                    $clauses[]  = $alterClause . ' SET' . $defaultClause;
                 }
             } else {
-                $clauses[] = $alterClause . ' DROP DEFAULT';
+                $needsReorg = true;
+                $clauses[]  = $alterClause . ' DROP DEFAULT';
             }
         }
 

diff --git a/src/Schema/TableDiff.php b/src/Schema/TableDiff.php
@@ -279,7 +279,7 @@ public function getModifiedColumns(): array
         Deprecation::triggerIfCalledFromOutside(
             'doctrine/dbal',
             'https://github.com/doctrine/dbal/pull/6080',
-            '%s is deprecated, use `getModifiedColumns()` instead.',
+            '%s is deprecated, use `getChangedColumns()` instead.',
             __METHOD__,
         );
 
@@ -301,7 +301,7 @@ public function getDroppedColumns(): array
     }
 
     /**
-     * @deprecated Use {@see getModifiedColumns()} instead.
+     * @deprecated Use {@see getChangedColumns()} instead.
      *
      * @return array<string,Column>
      */
@@ -310,7 +310,7 @@ public function getRenamedColumns(): array
         Deprecation::triggerIfCalledFromOutside(
             'doctrine/dbal',
             'https://github.com/doctrine/dbal/pull/6080',
-            '%s is deprecated, you should use `getModifiedColumns()` instead.',
+            '%s is deprecated, you should use `getChangedColumns()` instead.',
             __METHOD__,
         );
         $renamed = [];

diff --git a/tests/Platforms/DB2PlatformTest.php b/tests/Platforms/DB2PlatformTest.php
@@ -542,7 +542,8 @@ protected function getCommentOnColumnSQL(): array
     public function testGeneratesAlterColumnSQL(
         string $changedProperty,
         Column $column,
-        ?string $expectedSQLClause = null
+        ?string $expectedSQLClause = null,
+        bool $shouldReorg = true
     ): void {
         $tableDiff                        = new TableDiff('foo');
         $tableDiff->fromTable             = new Table('foo');
@@ -554,7 +555,9 @@ public function testGeneratesAlterColumnSQL(
             $expectedSQL[] = 'ALTER TABLE foo ALTER COLUMN bar ' . $expectedSQLClause;
         }
 
-        $expectedSQL[] = "CALL SYSPROC.ADMIN_CMD ('REORG TABLE foo')";
+        if ($shouldReorg) {
+            $expectedSQL[] = "CALL SYSPROC.ADMIN_CMD ('REORG TABLE foo')";
+        }
 
         self::assertSame($expectedSQL, $this->platform->getAlterTableSQL($tableDiff));
     }
@@ -612,6 +615,7 @@ public static function getGeneratesAlterColumnSQL(): iterable
                 'default',
                 new Column('bar', Type::getType(Types::INTEGER), ['autoincrement' => true, 'default' => 666]),
                 null,
+                false,
             ],
             [
                 'default',