Use proper check in acceptForeignKey() by greg0ire · Pull Request #3994 · doctrine/dbal

greg0ire · 2020-05-05T17:56:42Z

Q	A
Type	bug/feature/improvement
BC Break	no
Fixed issues	#3990

Summary

Checking that the platform supports foreign key is not the right case
here, because we should check if we can create them by themselves, after
the table is created. It is not the case for Sqlite.

Closes #3990

How can I test this?

composer config repositories.greg0ire vcs https://github.com/greg0ire/doctrine-dbal
composer require doctrine/dbal "dev-check-for-alter-support as 2.10.2"

lcobucci · 2020-05-05T18:54:36Z

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

    }
+
+    /**
+     * @doesNotPerformAssertions


Just a note: @doesNotPerformAssertions also disregards coverage (conscious decision on PHPUnit's side). The recommended way to test that an exception hasn't been thrown is calling $this->addToAssertionCount(1) after the method that throws exceptions.

@greg0ire sebastianbergmann/phpunit#3348 (and I didn't send the PR to fix the docs since I got no confirmation 😆)

I'm really confused about having @doesNotPerformAssertions here. Is the only outcome of actions in the test scenario is not having an exception thrown? I would understand such expectations in a validation scenario (no exception means “valid”) but not here.

@morozov my goal here was to have a failing test for an existing bug, I'm not really trying to assert anything, I want to fix a crash, and make sure it does not come back. Tell me if this file is not the right place for that (since it is in Functional, it might not be, but I'm not sure where I should put this). For PHPUnit I would put that kind of test here: For PHPUnit I would put it here: https://github.com/sebastianbergmann/phpunit/tree/master/tests/end-to-end/regression/GitHub

For DBAL… I have no clue. Please help.

I want to fix a crash, and make sure it does not come back.

IMO, the purpose of a test is to ensure that the code works, not that it doesn't crash. I.e. should be worded in terms of assertions against produced results.

How do you know that just the absence of failure is enough in the reported case?

Because this is what is complained about… they are not complaining that the fk does not get created properly for instance.

Anyway, I started by asserting that the generated SQL contains something about foreign key, but then I thought that even that might not work so I executed the SQL and asserted that there is a foreign key defined on it, tell me if I went overboard.

Because this is what is complained about… they are not complaining that the fk does not get created properly for instance.

I don't think this is the right mindset. But I believe we're on the same page now.

[…] tell me if I went overboard

The approach looks fine. The only question is, is this case is platform-specific or we can move it into a platform-independent suite and get covered on all platforms. The fact that people tend to write platform-specific tests because they experience issues using a given platform is the primary reason why we have inconsistency in platform support.

greg0ire · 2020-05-06T06:07:36Z

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

    }
+
+    public function testCreatingATableWithAForeignKey() : void
+    {


Few tests here have a guard clause about sqlite3, should I add one?

Is there anything specific to SQLite in this scenario? If yes, then I think the setup of the functional platform-specific test should require the corresponding platform to be active.

Otherwise, the test should be part of a platform-independent suite.

I think this is platform independent, and I'm not sure what a platform-independent suite is. I moved it to a file that inherits from DbalFunctionTestCase, let's see how this pans out. My only doubt is about the foreign key name, I have no idea what computes it and if it will be the same for every platform.

greg0ire · 2020-05-06T17:31:30Z

Just pushed a version with a way, way less specific assertion.

greg0ire · 2020-05-06T17:47:02Z

@morozov any idea why it fails for IBM DB2 and only for that RDBMS?

morozov · 2020-05-06T19:41:59Z

[…] any idea why it fails for IBM DB2 and only for that RDBMS?

I cannot check it right now. It's fine to mark it incomplete for DB2 and fix later but I think you'll need the @beberlei’s approval to get it merged.

morozov · 2020-05-06T19:59:40Z

Well, actually, the problem seems to be in the case-sensitivity of DB2 when using quoted names.

This is what the SQL creating the tables looks like (note that one name is quoted and the other isn't):

CREATE TABLE referenced (id INTEGER NOT NULL, PRIMARY KEY(id))
CREATE TABLE "referencing" (referenced_id INTEGER NOT NULL)
-- ...
ALTER TABLE "referencing" ADD CONSTRAINT FK_E844F4C85A38C91 FOREIGN KEY (referenced_id) REFERENCES referenced (id)

And then how the FK is selected:

dbal/lib/Doctrine/DBAL/Platforms/DB2Platform.php

Line 393 in d6f232e

WHERE fk.TABNAME = UPPER(" . $table . ')

I don't think that quoting object names by default on DB2 or Oracle is the right thing to do, so it's definitely a bug in DBAL.

And this is why it's quoted:

dbal/lib/Doctrine/DBAL/Platforms/Keywords/DB2Keywords.php

Line 370 in 72b4e77

'REFERENCING',

greg0ire · 2020-05-06T21:12:50Z

Ok, I'll just pick another name for this PR I guess! Thanks a lot for the investigation.

Checking that the platform supports foreign key is not the right case here, because we should check if we can create them by themselves, after the table is created. It is not the case for Sqlite. Closes doctrine#3990

greg0ire · 2020-05-07T05:50:44Z

The tests that fails is unrelated (and seems time based)

This reverts commit d1953d6. doctrine/dbal has been fixed, see doctrine/dbal#3994

…ire) This PR was squashed before being merged into the 3.4 branch. Discussion ---------- Remove patches for Doctrine bugs and deprecations | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no  | Deprecations? | no  | Tickets | n/a | License | MIT | Doc PR | n/a This PR removes patches put in place because of a bug fixed in doctrine/dbal#3994, and because of deprecations addressed in doctrine/orm#7953 Commits ------- 2f305cd Remove patches for Doctrine bugs and deprecations

greg0ire requested review from beberlei and morozov May 5, 2020 17:56

greg0ire added the Bug label May 5, 2020

This comment has been minimized.

Sign in to view

greg0ire force-pushed the check-for-alter-support-fk branch from 754e607 to e615ecf Compare May 5, 2020 17:58

lcobucci reviewed May 5, 2020

View reviewed changes

greg0ire force-pushed the check-for-alter-support-fk branch from e615ecf to 747101a Compare May 5, 2020 19:03

jrushlow mentioned this pull request May 5, 2020

[CI][Doctrine-Related] Functional tests failing on master branch across PHP 7.2 - 7.4 SymfonyCasts/reset-password-bundle#109

Closed

greg0ire force-pushed the check-for-alter-support-fk branch 2 times, most recently from 80708dd to a97799f Compare May 6, 2020 06:06

greg0ire commented May 6, 2020

View reviewed changes

greg0ire added this to the 2.10.3 milestone May 6, 2020

greg0ire force-pushed the check-for-alter-support-fk branch 3 times, most recently from 86eea63 to fe17629 Compare May 6, 2020 17:31

Use proper check in acceptForeignKey()

f0c1af4

Checking that the platform supports foreign key is not the right case here, because we should check if we can create them by themselves, after the table is created. It is not the case for Sqlite. Closes doctrine#3990

greg0ire force-pushed the check-for-alter-support-fk branch from fe17629 to f0c1af4 Compare May 6, 2020 21:16

morozov approved these changes May 6, 2020

View reviewed changes

beberlei approved these changes May 7, 2020

View reviewed changes

greg0ire closed this May 7, 2020

greg0ire reopened this May 7, 2020

greg0ire merged commit f20ba14 into doctrine:2.10.x May 7, 2020

greg0ire deleted the check-for-alter-support-fk branch May 7, 2020 07:07

greg0ire self-assigned this May 7, 2020

greg0ire added a commit to greg0ire/symfony that referenced this pull request May 7, 2020

Revert "Force doctrine/dbal <=2.10.2 when testing"

1b0dbda

This reverts commit d1953d6. doctrine/dbal has been fixed, see doctrine/dbal#3994

greg0ire mentioned this pull request May 7, 2020

Remove patches for Doctrine bugs and deprecations symfony/symfony#36744

Merged

This was referenced May 17, 2020

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

Closed

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

Closed

greg0ire mentioned this pull request Jun 4, 2020

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

Closed

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

Uh oh!

Conversation

greg0ire commented May 5, 2020

Summary

How can I test this?

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented May 6, 2020

Uh oh!

greg0ire commented May 6, 2020

Uh oh!

morozov commented May 6, 2020

Uh oh!

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

Uh oh!

Uh oh!

greg0ire commented May 6, 2020

Uh oh!

greg0ire commented May 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lcobucci May 5, 2020 •

edited

Loading

greg0ire May 5, 2020 •

edited

Loading

morozov May 6, 2020 •

edited

Loading

morozov commented May 6, 2020 •

edited

Loading