Fix a condition that would not allow diffing of foreign keys for Sqlite by camilleterol · Pull Request #4012 · doctrine/dbal

camilleterol · 2020-05-17T11:32:18Z

Q	A
Type	bug
BC Break	yes
Fixed issues	#4010

Summary

After the changes introduced by #1204, the SchemaDiff would throw an error when trying to diff an Sqlite-backed schema with foreign keys modifications.

camilleterol · 2020-05-17T11:33:39Z

For safe-keeping, here is the previous PR that was not referencing the correct branch.

greg0ire · 2020-05-17T12:03:46Z

For safe-keeping, here is the previous PR that was not referencing the correct branch.

FYI you could have just clicked the Edit button and changed your target. But that would have been confusing because of how you named your branch (or rather, did not rename it). Make sure to rename it in your future contributions 🙏

greg0ire · 2020-05-17T12:06:00Z

Copying my comment here:

This looks a lot like #3994 … maybe there are more occurrences of supportsForeignKeyConstraints that should be re-evaluated?

In any case, please add a test.

camilleterol · 2020-05-17T12:06:24Z

Sorry about that. I forked the whole thing instead of creating a branch for the PR because I'm lazy.

greg0ire · 2020-05-17T12:07:35Z

It's not SVN anymore, creating branches is cheap, try it next time ;)

camilleterol · 2020-05-17T12:11:31Z

For sure! I haven't used SVN since 2008 so creating a branch is what I usually do. But I was confused by GitHub's UI.

camilleterol · 2020-05-21T10:27:37Z

In any case, please add a test.

Today being a bank holiday, I'll try and add the test case that we mentioned above for SQLite.

Approaching the repository, though, I'm wondering if it could be of use to add a CONTRIBUTE guide and a section on how to run the tests in the README to lower the barrier of entry.

greg0ire · 2020-05-21T11:19:27Z

Approaching the repository, though, I'm wondering if it could be of use to add a CONTRIBUTE guide and a section on how to run the tests in the README to lower the barrier of entry.

I have had the same thought in the back of my mind, I know @morozov asks for a lot of functional tests, but when I contributed as a dev rather than as a maintainer in #3994, I had a hard time figuring out where this should go and how things are organized.

@morozov , your input on this would be priceless.

camilleterol · 2020-05-21T11:59:27Z

I wrote a failing test, then made sure that my modification in commit dd6aaea would make the test pass. TDD 🎉

greg0ire · 2020-05-21T12:03:55Z

tests/Doctrine/Tests/DBAL/Schema/SqliteSchemaDiffTest.php

+        $diff     = $this->createSchemaDiff();
+        $platform = $this->createPlatform(true);
+
+        $sql = $diff->toSql($platform);
+
+        $expected = ['create_table'];
+
+        self::assertEquals($expected, $sql);


Suggested change

$diff = $this->createSchemaDiff();

$platform = $this->createPlatform(true);

$sql = $diff->toSql($platform);

$expected = ['create_table'];

self::assertEquals($expected, $sql);

self::assertEquals(

['create_table'],

$this->createSchemaDiff()->toSql($this->createPlatform(true))

);

greg0ire · 2020-05-21T12:05:36Z

tests/Doctrine/Tests/DBAL/Schema/SqliteSchemaDiffTest.php

+
+        $expected = ['create_table'];
+
+        self::assertEquals($expected, $sql);


This looks exactly as the test above. Instead of having the reader read both tests to notice that, please use one test with a data provider. The data provider should have keys describing what true and false mean.

greg0ire · 2020-05-21T12:06:35Z

tests/Doctrine/Tests/DBAL/Schema/SqliteSchemaDiffTest.php

+    /**
+     * @return AbstractPlatform|MockObject
+     */
+    private function createPlatform(bool $unsafe)


Can be merged with the aforementioned test I suppose

greg0ire · 2020-05-21T12:08:01Z

tests/Doctrine/Tests/DBAL/Schema/SqliteSchemaDiffTest.php

+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+class SqliteSchemaDiffTest extends TestCase


Are the tests specific to Sqlite or could they apply to other platforms?

I'm not sure. They are testing a specific use case that I encountered with SQLite, it might apply to any platform where supportsForeignKeyConstraints() is true and supportsCreateDropForeignKeyConstraints() is false.

Given the number of things mocked, I can barely understand what is being tested here. Would it make sense to have an integration test instead?

I guess so. I just copied the code that was in the SchemaDiffTest. Maybe a more generic integration test could cover both cases.

greg0ire · 2020-05-21T12:09:03Z

tests/Doctrine/Tests/DBAL/Schema/SqliteSchemaDiffTest.php

+                     ->will($this->throwException(
+                         new DBALException('Sqlite platform does not support alter foreign key, the table must be fully recreated using getAlterTableSQL.')
+                     ));
+        }


That's a lot of mocking… why not use the real object? A cross-platform functional test should make all this a bit shorter.

My thoughts exactly. All of your above remarks apply. I just copied the SchemaDiffTest and kept the same coding style.

camilleterol · 2020-05-21T13:41:29Z

I tweaked my test case a little. My time working on this for today is up. But any feedback is welcome :) I might be able to fork on this further if need be tomorrow or Sunday.

camilleterol · 2020-05-23T09:54:21Z

I'll have some more time to work on this tomorrow, so what do you guys prefer? Should I strive for consistency and keep the test somewhat identical to the SchemaDiffTest. Or should I refactor this test and the SchemaDiffTest, use platform classes instead of a mock and not target SQLite specifically, but all platforms that have supportsForeignKeyConstraints() === true and supportsCreateDropForeignKeyConstraints() === false?

greg0ire · 2020-05-23T13:59:56Z

I'll have some more time to work on this tomorrow, so what do you guys prefer? Should I strive for consistency and keep the test somewhat identical to the SchemaDiffTest.

I don't think you should, or maybe be consistent with another, better test if you can find one.

but all platforms that have supportsForeignKeyConstraints() === true and supportsCreateDropForeignKeyConstraints() === false

Sounds good. You should be able to do so by using $this->markTestSkipped() in a condition.

camilleterol · 2020-05-30T13:31:43Z

Thanks for your insight on the matter. I'll do just that. Unfortunately I have not gotten the time yet to work on this further. I will try and take a look at it in the next few days if time allows.

camilleterol added 3 commits May 21, 2020 13:44

Fix a condition that would not allow diffing of foreign keys for Sqlite

dd6aaea

Invert a condition to fix the test suite on macOS

43dd601

Add a test to check the proper diffing of foreign keys with SQLite

c4b463e

camilleterol force-pushed the 2.10.x branch from b841342 to c4b463e Compare May 21, 2020 11:45

Fix the style to comply with phpcs rules

714f4c5

greg0ire reviewed May 21, 2020

View reviewed changes

greg0ire requested a review from morozov May 21, 2020 12:25

camilleterol added 2 commits May 21, 2020 15:03

Refactor the tests to use a dataProvider

2341e1c

Fix the coding style

80aa6c2

greg0ire mentioned this pull request Jun 4, 2020

Only pass the additional arguments if they are not null #3984

Closed

morozov removed their request for review October 23, 2020 16:51

morozov changed the base branch from 2.10.x to 2.12.x November 29, 2020 21:34

morozov closed this Apr 21, 2021

morozov deleted the branch doctrine:2.12.x April 21, 2021 16:25

github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2022


		$expected = ['create_table'];

		self::assertEquals($expected, $sql);

Uh oh!

Conversation

camilleterol commented May 17, 2020

Summary

Uh oh!

camilleterol commented May 17, 2020

Uh oh!

greg0ire commented May 17, 2020

Uh oh!

greg0ire commented May 17, 2020

Uh oh!

camilleterol commented May 17, 2020

Uh oh!

greg0ire commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilleterol commented May 17, 2020

Uh oh!

camilleterol commented May 21, 2020

Uh oh!

greg0ire commented May 21, 2020

Uh oh!

camilleterol commented May 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilleterol commented May 21, 2020

Uh oh!

camilleterol commented May 23, 2020

Uh oh!

greg0ire commented May 23, 2020

Uh oh!

camilleterol commented May 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greg0ire commented May 17, 2020 •

edited

Loading