Fix BC-break regarding JsonArrayType #2855

lcobucci · 2017-09-11T09:50:20Z

As reported on #2782 (comment) we've introduced a BC-break on 2.6.x regarding the json_array type. This fixes it for good.

Ocramius · 2017-09-12T05:18:46Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+     */
+    public function testComparatorShouldReturnFalseWhenLegacyJsonArrayColumnHasComment()
+    {
+        if ( ! $this->_sm->getDatabasePlatform()->hasNativeJsonType()) {


Doesn't this test also run on anything with TEXT support?

Ocramius · 2017-09-12T05:20:02Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        self::assertFalse($tableDiff);
+
+        $this->_sm->dropTable('json_array_test');


Should be done in tearDown or such: otherwise, a failure in this test can lead to other failures in colliding table names in tests

Ocramius · 2017-09-12T05:20:42Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        $this->_sm->dropTable('json_array_test');


Should be done in tearDown or such: otherwise, a failure in this test can lead to other failures in colliding table names in tests

Ocramius · 2017-09-12T05:20:49Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_array_test'), $table);
+
+        $this->_sm->dropTable('json_array_test');


Should be done in tearDown or such: otherwise, a failure in this test can lead to other failures in colliding table names in tests

Ocramius · 2017-09-12T05:21:02Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        $comparator = new Comparator();
+        $tableDiff  = $comparator->diffTable($this->_sm->listTableDetails('json_test'), $table);
+        $this->_sm->dropTable('json_test');


Should be done in tearDown or such: otherwise, a failure in this test can lead to other failures in colliding table names in tests

Ocramius · 2017-09-12T05:27:02Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        }
+
+        $table = new Table('json_array_test');
+        $table->addColumn('id', 'integer');


Can you drop columns that aren't strictly required?

Ocramius · 2017-09-12T05:27:14Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        );
+
+        $table = new Table('json_array_test');
+        $table->addColumn('id', 'integer');


Can you drop columns that aren't strictly required?

Ocramius · 2017-09-12T05:27:25Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        );
+
+        $table = new Table('json_array_test');
+        $table->addColumn('id', 'integer');


Can you drop columns that aren't strictly required?

Ocramius · 2017-09-12T05:27:34Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        );
+
+        $table = new Table('json_test');
+        $table->addColumn('id', 'integer');


Can you drop columns that aren't strictly required?

Ocramius · 2017-09-12T05:30:42Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        }
+
+        $this->_conn->executeQuery(
+            'CREATE TABLE json_test (id INT NOT NULL, parameters JSON NOT NULL, PRIMARY KEY(id))'


Is this because you need to reproduce half-migrated column definitions?

Yeap, that ensures we won't break things baddly

lcobucci · 2017-10-26T09:17:25Z

@Ocramius @deeky666 @alcaeus any idea on why Oracle is failing on that way? I mean, the table has only one column and it's not part of the changedProperties WTF?

On 31d0128 we made JsonType the default one, which caused some unexpected results with the comparator. This tests validates that if users have a JSON column but are using the JsonArrayType we'll just add a comment to the column. Reference: doctrine#2782

And then we don't need to wait till v3 to not require comments on the JsonType.

lcobucci · 2017-11-18T20:34:44Z

Ok, I've found out that Oracle returns the modified columns using uppercase instead... tests updated and hopefully everything will work fine

Requested changes have been applied

I know, this is REALLY nasty but that was the only way I've found to make it work properly.

Backporting doctrine#2855

lcobucci added the BC Fix label Sep 11, 2017

lcobucci added this to the 2.6.3 milestone Sep 11, 2017

lcobucci self-assigned this Sep 11, 2017

lcobucci requested review from Majkl578, Ocramius and deeky666 September 11, 2017 09:50

lcobucci added the WIP label Sep 11, 2017

This was referenced Sep 11, 2017

Stop using the deprecated json_array type internally #2782

Merged

Problem with mapper doctrine/orm#6654

Open

lcobucci force-pushed the bc-break/fix-json_array-fields branch 2 times, most recently from 18339f5 to 6937f25 Compare September 11, 2017 09:58

lcobucci mentioned this pull request Sep 11, 2017

doctrine schema diff generates continues diff with json fields in 1.7 doctrine/migrations#546

Closed

Ocramius previously requested changes Sep 12, 2017

View reviewed changes

Ocramius mentioned this pull request Sep 12, 2017

CURRENT_TIMESTAMP not possible anymore as default value doctrine/orm#6698

Closed

lcobucci mentioned this pull request Sep 18, 2017

The database schema is not in sync at once command d:s:create is finished doctrine/DoctrineBundle#696

Closed

lcobucci mentioned this pull request Oct 8, 2017

Symfony 3 - Postgresql JSON type always appear in diff #2880

Closed

lcobucci force-pushed the bc-break/fix-json_array-fields branch from 6937f25 to 9058153 Compare October 25, 2017 18:19

lcobucci requested a review from Ocramius October 25, 2017 18:19

lcobucci force-pushed the bc-break/fix-json_array-fields branch from 9058153 to 3b5c771 Compare November 18, 2017 20:26

lcobucci added 2 commits November 18, 2017 21:31

Force comments on the JsonArrayType

e11d799

And then we don't need to wait till v3 to not require comments on the JsonType.

lcobucci force-pushed the bc-break/fix-json_array-fields branch from 3b5c771 to c90b26e Compare November 18, 2017 20:32

lcobucci changed the title ~~[WIP] Fix BC-break regarding JsonArrayType~~ Fix BC-break regarding JsonArrayType Nov 18, 2017

lcobucci removed the WIP label Nov 18, 2017

Force comparator to add comment (json -> json_array)

104793c

I know, this is REALLY nasty but that was the only way I've found to make it work properly.

lcobucci force-pushed the bc-break/fix-json_array-fields branch from c90b26e to 104793c Compare November 18, 2017 21:18

lcobucci merged commit 39cb21b into doctrine:master Nov 18, 2017

lcobucci deleted the bc-break/fix-json_array-fields branch November 18, 2017 21:31

lcobucci added a commit to lcobucci/dbal that referenced this pull request Nov 18, 2017

Merge branch 'bc-break/fix-json_array-fields' into 2.6

1595a43

Backporting doctrine#2855

lcobucci added Schema Comparison Type Mapping Types labels Nov 18, 2017

lcobucci mentioned this pull request Nov 26, 2017

Type names with backslashes cause infinite upgrades doctrine/orm#6752

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BC-break regarding JsonArrayType #2855

Fix BC-break regarding JsonArrayType #2855

lcobucci commented Sep 11, 2017

Ocramius Sep 12, 2017

Ocramius Sep 12, 2017

Ocramius Sep 12, 2017

Ocramius Sep 12, 2017

Ocramius Sep 12, 2017

Ocramius Sep 12, 2017

lcobucci Sep 18, 2017

Ocramius Sep 12, 2017

lcobucci Sep 18, 2017

Ocramius Sep 12, 2017

lcobucci Sep 18, 2017

Ocramius Sep 12, 2017

lcobucci Sep 18, 2017

Ocramius Sep 12, 2017

lcobucci Sep 18, 2017

lcobucci commented Oct 26, 2017

lcobucci commented Nov 18, 2017


		self::assertFalse($tableDiff);

		$this->_sm->dropTable('json_array_test');

Fix BC-break regarding JsonArrayType #2855

Fix BC-break regarding JsonArrayType #2855

Conversation

lcobucci commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci commented Oct 26, 2017

lcobucci commented Nov 18, 2017