Merge pull request #2855 from lcobucci/bc-break/fix-json_array-fields

Fix BC-break regarding JsonArrayType Fixes: doctrine/DoctrineBundle#696 Fixes: #2880
doctrine · Nov 18, 2017 · 39cb21b · 39cb21b
2 parents c7757e3 + 104793c
commit 39cb21b
Show file tree

Hide file tree

Showing 5 changed files with 167 additions and 8 deletions.
diff --git a/lib/Doctrine/DBAL/Schema/Comparator.php b/lib/Doctrine/DBAL/Schema/Comparator.php
@@ -435,6 +435,13 @@ public function diffColumn(Column $column1, Column $column2)
             }
         }
 
+        // This is a very nasty hack to make comparator work with the legacy json_array type, which should be killed in v3
+        if ($this->isALegacyJsonComparison($properties1['type'], $properties2['type'])) {
+            array_shift($changedProperties);
+
+            $changedProperties[] = 'comment';
+        }
+
         if ($properties1['default'] != $properties2['default'] ||
             // 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.
@@ -497,6 +504,21 @@ public function diffColumn(Column $column1, Column $column2)
         return array_unique($changedProperties);
     }
 
+    /**
+     * TODO: kill with fire on v3.0
+     *
+     * @deprecated
+     */
+    private function isALegacyJsonComparison(Types\Type $one, Types\Type $other) : bool
+    {
+        if ( ! $one instanceof Types\JsonType || ! $other instanceof Types\JsonType) {
+            return false;
+        }
+
+        return ( ! $one instanceof Types\JsonArrayType && $other instanceof Types\JsonArrayType)
+            || ( ! $other instanceof Types\JsonArrayType && $one instanceof Types\JsonArrayType);
+    }
+
     /**
      * Finds the difference between the indexes $index1 and $index2.
      *

diff --git a/lib/Doctrine/DBAL/Types/JsonArrayType.php b/lib/Doctrine/DBAL/Types/JsonArrayType.php
@@ -57,6 +57,6 @@ public function getName()
      */
     public function requiresSQLCommentHint(AbstractPlatform $platform)
     {
-        return ! $platform->hasNativeJsonType();
+        return true;
     }
 }
diff --git a/lib/Doctrine/DBAL/Types/JsonType.php b/lib/Doctrine/DBAL/Types/JsonType.php
@@ -90,12 +90,7 @@ public function getName()
      */
     public function requiresSQLCommentHint(AbstractPlatform $platform)
     {
-        /*
-         * should be switched back to the platform detection at 3.0, when
-         * JsonArrayType will be dropped
-         */
-        //return ! $platform->hasNativeJsonType();
-        return true;
+        return ! $platform->hasNativeJsonType();
     }
 
     /**

diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php
@@ -383,7 +383,7 @@ public function testJsonbColumn(string $type): void
         /** @var Schema\Column[] $columns */
         $columns = $this->_sm->listTableColumns('test_jsonb');
 
-        self::assertSame(TYPE::JSON, $columns['foo']->getType()->getName());
+        self::assertSame($type, $columns['foo']->getType()->getName());
         self::assertTrue(true, $columns['foo']->getPlatformOption('jsonb'));
     }
 

diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
@@ -1192,4 +1192,146 @@ public function testDoesNotListIndexesImplicitlyCreatedByForeignKeys()
         self::assertArrayHasKey('explicit_fk1_idx', $indexes);
         self::assertArrayHasKey('idx_3d6c147fdc58d6c', $indexes);
     }
+
+    /**
+     * @after
+     */
+    public function removeJsonArrayTable() : void
+    {
+        if ($this->_sm->tablesExist(['json_array_test'])) {
+            $this->_sm->dropTable('json_array_test');
+        }
+    }
+
+    /**
+     * @group 2782
+     * @group 6654
+     */
+    public function testComparatorShouldReturnFalseWhenLegacyJsonArrayColumnHasComment() : void
+    {
+        $table = new Table('json_array_test');
+        $table->addColumn('parameters', 'json_array');
+
+        $this->_sm->createTable($table);
+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        self::assertFalse($tableDiff);
+    }
+
+    /**
+     * @group 2782
+     * @group 6654
+     */
+    public function testComparatorShouldModifyOnlyTheCommentWhenUpdatingFromJsonArrayTypeOnLegacyPlatforms() : void
+    {
+        if ($this->_sm->getDatabasePlatform()->hasNativeJsonType()) {
+            $this->markTestSkipped('This test is only supported on platforms that do not have native JSON type.');
+        }
+
+        $table = new Table('json_array_test');
+        $table->addColumn('parameters', 'json_array');
+
+        $this->_sm->createTable($table);
+
+        $table = new Table('json_array_test');
+        $table->addColumn('parameters', 'json');
+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        self::assertInstanceOf(TableDiff::class, $tableDiff);
+
+        $changedColumn = $tableDiff->changedColumns['parameters'] ?? $tableDiff->changedColumns['PARAMETERS'];
+
+        self::assertSame(['comment'], $changedColumn->changedProperties);
+    }
+
+    /**
+     * @group 2782
+     * @group 6654
+     */
+    public function testComparatorShouldAddCommentToLegacyJsonArrayTypeThatDoesNotHaveIt() : void
+    {
+        if ( ! $this->_sm->getDatabasePlatform()->hasNativeJsonType()) {
+            $this->markTestSkipped('This test is only supported on platforms that have native JSON type.');
+        }
+
+        $this->_conn->executeQuery('CREATE TABLE json_array_test (parameters JSON NOT NULL)');
+
+        $table = new Table('json_array_test');
+        $table->addColumn('parameters', 'json_array');
+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        self::assertInstanceOf(TableDiff::class, $tableDiff);
+        self::assertSame(['comment'], $tableDiff->changedColumns['parameters']->changedProperties);
+    }
+
+    /**
+     * @group 2782
+     * @group 6654
+     */
+    public function testComparatorShouldReturnAllChangesWhenUsingLegacyJsonArrayType() : void
+    {
+        if ( ! $this->_sm->getDatabasePlatform()->hasNativeJsonType()) {
+            $this->markTestSkipped('This test is only supported on platforms that have native JSON type.');
+        }
+
+        $this->_conn->executeQuery('CREATE TABLE json_array_test (parameters JSON DEFAULT NULL)');
+
+        $table = new Table('json_array_test');
+        $table->addColumn('parameters', 'json_array');
+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        self::assertInstanceOf(TableDiff::class, $tableDiff);
+        self::assertSame(['notnull', 'comment'], $tableDiff->changedColumns['parameters']->changedProperties);
+    }
+
+    /**
+     * @group 2782
+     * @group 6654
+     */
+    public function testComparatorShouldReturnAllChangesWhenUsingLegacyJsonArrayTypeEvenWhenPlatformHasJsonSupport() : void
+    {
+        if ( ! $this->_sm->getDatabasePlatform()->hasNativeJsonType()) {
+            $this->markTestSkipped('This test is only supported on platforms that have native JSON type.');
+        }
+
+        $this->_conn->executeQuery('CREATE TABLE json_array_test (parameters JSON DEFAULT NULL)');
+
+        $table = new Table('json_array_test');
+        $table->addColumn('parameters', 'json_array');
+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        self::assertInstanceOf(TableDiff::class, $tableDiff);
+        self::assertSame(['notnull', 'comment'], $tableDiff->changedColumns['parameters']->changedProperties);
+    }
+
+    /**
+     * @group 2782
+     * @group 6654
+     */
+    public function testComparatorShouldNotAddCommentToJsonTypeSinceItIsTheDefaultNow() : void
+    {
+        if ( ! $this->_sm->getDatabasePlatform()->hasNativeJsonType()) {
+            $this->markTestSkipped('This test is only supported on platforms that have native JSON type.');
+        }
+
+        $this->_conn->executeQuery('CREATE TABLE json_test (parameters JSON NOT NULL)');
+
+        $table = new Table('json_test');
+        $table->addColumn('parameters', 'json');
+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_test'), $table);
+
+        self::assertFalse($tableDiff);
+    }
 }