Skip to content

Populate foreign key constraint options only with non-default values#6964

Merged
morozov merged 2 commits intodoctrine:4.3.xfrom
morozov:foreign-key-constraint-default-options
May 20, 2025
Merged

Populate foreign key constraint options only with non-default values#6964
morozov merged 2 commits intodoctrine:4.3.xfrom
morozov:foreign-key-constraint-default-options

Conversation

@morozov
Copy link
Member

@morozov morozov commented May 20, 2025

The current implementation if ForeignKeyConstraintEditor::create() populates the match, onUpdate and onDelete options of the foreign key constraints even if they weren't explicitly set and thus are equal to their default values.

This is correct – the default options match to the corresponding defaults in SQL. The "problem" is that if a table is constructed via the editor, its CREATE TABLE SQL will explicitly contain the default clauses like ON UPDATE NO ACTION. Functionally, this is still not a problem, I just want to keep the behavior of the editor such that switching the implementation of the table construction doesn't change the generated SQL.

The second problem is auto-generated index names for quoted columns. This issue is tracked in #6733 – the resulting auto-generated name depends on the quote character used to declare a quoted name. Since TableEditor internally uses Identifier::toString() which in turn uses double quotes, switching from new Table() with backticks to TableEditor would result in different auto-generated name. This is inevitable. I've extracted this into a separate commit to demonstrate that this is an existing issue, and the change is test-only.

morozov added 2 commits May 19, 2025 16:11
This change indicates the issue being tracked in XYZ. It is extracted
into a separate commit to demonstrate the issue and the fact that this
is a test-only change.
@morozov morozov force-pushed the foreign-key-constraint-default-options branch from 13501ad to 95039c4 Compare May 20, 2025 12:59
@morozov morozov added this to the 4.3.0 milestone May 20, 2025
@morozov morozov marked this pull request as ready for review May 20, 2025 16:39
@morozov morozov requested a review from greg0ire May 20, 2025 16:39
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand, in 95039c4 you wanted to use the ForeignKeyConstraintEditor in a test, but couldn't because it generated different SQL than the previous implementation, is that correct? If yes this looks OK to me.

@morozov
Copy link
Member Author

morozov commented May 20, 2025

Yes. If 95039c4 is reverted, the reworked test will fail as follows:

There were 9 failures:

1) Doctrine\DBAL\Tests\Platforms\DB2PlatformTest::testQuotedColumnInForeignKeyPropagation
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     1 => 'ALTER TABLE "quoted" ADD CONS...-bar")'
     2 => 'ALTER TABLE "quoted" ADD CONS...-bar")'
     3 => 'ALTER TABLE "quoted" ADD CONS...-bar")'
-    4 => '..._22660D028FD6E0FB8C736521D79164E3 ON "quoted" ("create", foo, "bar")'
+    4 => '..._22660D028FD6E0FB8C7365216D704F76 ON "quoted" ("create", foo, "bar")'
 )

/Users/morozov/Projects/dbal/tests/Platforms/AbstractPlatformTestCase.php:480

@morozov morozov merged commit 00b621a into doctrine:4.3.x May 20, 2025
82 checks passed
@morozov morozov deleted the foreign-key-constraint-default-options branch May 20, 2025 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants