Deprecate unnamed foreign key constraints in `$droppedForeignKeys` of `TableDiff` by santysisi · Pull Request #7143 · doctrine/dbal

santysisi · 2025-09-05T00:15:22Z

Q	A
Type	bug
Fixed issues	#7124

Summary

This PR introduces a deprecation notice when a unnamed foreign key constraints in $droppedForeignKeys is passed in the constructor of TableDiff.

santysisi · 2025-09-05T00:17:09Z

I added the test to SchemaManagerFunctionalTestCase instead of SQLiteSchemaManagerTest because I believe it's relevant for all platforms, not just SQLite.
However, if this test should be scoped to SQLite only, please let me know and I’ll move it accordingly.

morozov · 2025-09-10T01:54:00Z

src/Platforms/SQLitePlatform.php


-        foreach ($diff->getDroppedForeignKeys() as $constraint) {
-            $constraintName = $constraint->getName();
+        foreach ($diff->getDroppedForeignKeys() as $name => $constraint) {


This is against the effort made in #7102. The constraint name is represented by the constraint object itself, not by some array key.

Got it, thanks for pointing that out! I’ll look into applying a different approach that aligns better with the changes from #7102 as soon as possible.

It might be a good idea to just deprecate the scenario when the constraint being dropped doesn't have a name. Currently, we're not doing anything, which causes a bug. Making it work correctly would require a deeper redesign of schema management.

Just for context, the fundamental flaw is the following. The test attempts to perform a schema migration in the following steps:

Define two versions of a table.

Calculate a difference between them.

Apply the difference.

The problem is that:

The calculation of the difference is non-deterministic (you cannot reliably tell whether a given column was renamed or dropped and created). This doesn't apply to this specific case, but still.

SQLite doesn't support ALTER TABLE. The DBAL has to recreate it from scratch and migrate the data.

The combination of these flaws leads to the fact that we cannot migrate schemas with unnamed constraints. A proper design for SQLite would not involve the diff at all. For SQLite, it would use the expected table schema directly.

Hi, thanks a lot for the detailed explanation! 😊
That makes sense. I’ll update the PR this weekend to only emit a deprecation notice in this case.

Thanks again! ❤️

santysisi · 2025-09-14T23:57:42Z

Hi! 😄
I've updated the PR to trigger a deprecation. Additionally, I've changed the base branch, as well as the PR title and description.

santysisi · 2025-09-15T21:41:25Z

Hi @morozov, I hope you're doing well! 😄

I've updated this PR based on your suggestions from the previous one. I modified the deprecation message, updated the UPGRADE.md, and removed unnecessary code in the test.

Please let me know if anything needs to be changed or corrected 😊

Thanks again! ❤️

morozov · 2025-09-16T18:18:22Z

UPGRADE.md


+## Deprecated dropping unnamed constraints in SQLite
+
+Dropping a constraint without specifying its name has been deprecated on the SQLite platform.


This will have to be reworded accordingly. Something like this:

Passing unnamed foreign key constraints as part of the $droppedForeignKeys argument of the TableDiff constructor has been deprecated.

morozov · 2025-09-16T18:19:19Z

src/Platforms/SQLitePlatform.php

            $constraintName = $constraint->getName();

            if ($constraintName === '') {
+                Deprecation::trigger(


Whether a foreign key constraint is dropped on SQLite or any other platform, it's expected to have a name:

dbal/src/Platforms/AbstractPlatform.php

Lines 1372 to 1374 in aa6b906

foreach ($diff->getDroppedForeignKeys() as $foreignKey) {

$sql[] = $this->getDropForeignKeySQL($foreignKey->getQuotedName($this), $tableNameSQL);

}

The fact that TableDiff::getDroppedForeignKeys() can return unnamed constraints is essentially a bug as using it as is will result in invalid DDL.

I think we should move this deprecation to the TableDiff constructor. In 5.0.x, this deprecation will need to be promoted to an exception, and all callers of TableDiff::getDroppedForeignKeys() could make an assertion that the constraint being dropped does have a name before using it.

Hi! 😄
Thanks so much for the suggestions ❤️

I’ve made the following changes in the test:

Updated the test a bit, it now verifies that the deprecation is triggered in the constructor of TableDiff.

Also, it checks that a foreign key is not dropped when the dropped foreign key doesn't have a name.

Moved the test from SQLiteSchemaManagerTest to SchemaManagerFunctionalTestCase so it runs across all platforms.

I do have one question. I’m still getting familiar with Doctrine DBAL 😅

Do you think this check is necessary?

if ($constraintName === null || $constraintName === '') {

Or would just checking $constraintName === null be enough? 🤔

Thanks again for your help! 😊

morozov · 2025-09-17T00:14:00Z

src/Platforms/AbstractPlatform.php

        foreach ($diff->getDroppedForeignKeys() as $foreignKey) {
+            $constraintName = $foreignKey->getObjectName()?->toString();
+            if ($constraintName === null || $constraintName === '') {
+                continue;


We don't want to do this. It is better to have invalid DDL generated, so the user sees that something is wrong, rather then get an invalid input silently ignored.

morozov · 2025-09-17T00:16:52Z

src/Schema/TableDiff.php

+            $constraintName = $droppedForeignKey->getObjectName()?->toString();
+            if ($constraintName === null || $constraintName === '') {


We cannot use getObjectName() in the library code in 4.4.x since it can throw an exception, if the name is invalid and cannot be parsed using the 5.0.x logic, but can be using the 4.4.x one. Please use if ($droppedForeignKey->getName() === '') instead.

morozov · 2025-09-17T00:17:27Z

tests/Functional/Schema/SchemaManagerFunctionalTestCase.php

        self::assertSame(2, $columns['column_char']->getLength());
    }

+    public function testAlterTableWithUnnamedForeignKey(): void


Please replace this with a TableDiff constructor test.

morozov · 2025-09-17T03:54:12Z

tests/Schema/TableDiffTest.php

+            ->create();
+
+        $droppedForeignKeys = ForeignKeyConstraint::editor()
+            ->setUnquotedReferencingColumnNames('c1', 'c2')


c2 can be removed from the definition: it's not needed to reproduce the issue and it doesn't exist on the referencing table.

… `TableDiff` DroppedForeignKey must have a constraint name to ensure reliable schema diffing and migration generation. A deprecation warning is now triggered if the name is missing.

morozov · 2025-09-17T12:41:39Z

Thanks, @santysisi!

andersonamuller · 2025-09-18T08:54:11Z

UPGRADE.md


 # Upgrade to 4.4

+## Deprecated dropping unnamed constraints in SQLite


@santysisi Does it still only apply for SQLite?

Suggested change

## Deprecated dropping unnamed constraints in SQLite

## Deprecated dropping unnamed constraints

You're right, that was my mistake.
Thanks so much for catching that! 🙏
I've opened this PR to resolve the issue

derrabus added Bug Schema Management labels Sep 5, 2025

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch from 54bae1a to c2852f3 Compare September 5, 2025 01:11

morozov reviewed Sep 10, 2025

View reviewed changes

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch from c2852f3 to 3b3b7ca Compare September 14, 2025 23:51

santysisi changed the base branch from 4.3.x to 4.4.x September 14, 2025 23:52

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch from 3b3b7ca to c809db7 Compare September 14, 2025 23:55

santysisi changed the title ~~Fix handling of unnamed foreign key drops in SQLite~~ Deprecate unnamed DroppedForeignKey constraints Sep 14, 2025

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch 4 times, most recently from e774f56 to a656527 Compare September 15, 2025 21:39

santysisi changed the title ~~Deprecate unnamed DroppedForeignKey constraints~~ Deprecated dropping unnamed constraints in SQLite Sep 15, 2025

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch from a656527 to 3e16542 Compare September 15, 2025 21:59

morozov reviewed Sep 16, 2025

View reviewed changes

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch from 3e16542 to cdb1a22 Compare September 16, 2025 23:36

morozov reviewed Sep 17, 2025

View reviewed changes

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch 2 times, most recently from bbfeaff to 77a9f60 Compare September 17, 2025 00:50

santysisi changed the title ~~Deprecated dropping unnamed constraints in SQLite~~ Deprecate unnamed foreign key constraints in $droppedForeignKeys of TableDiff Sep 17, 2025

morozov reviewed Sep 17, 2025

View reviewed changes

Deprecate unnamed foreign key constraints in $droppedForeignKeys of…

5d56d2e

… `TableDiff` DroppedForeignKey must have a constraint name to ensure reliable schema diffing and migration generation. A deprecation warning is now triggered if the name is missing.

santysisi force-pushed the fix/sqlite-drop-unnamed-foreign-keys branch from 77a9f60 to 5d56d2e Compare September 17, 2025 12:08

morozov approved these changes Sep 17, 2025

View reviewed changes

morozov merged commit 742df57 into doctrine:4.4.x Sep 17, 2025
96 checks passed

andersonamuller reviewed Sep 18, 2025

View reviewed changes

santysisi mentioned this pull request Sep 20, 2025

Drop deprecation in TableDiff and throw exception instead #7156

Merged

morozov mentioned this pull request Sep 20, 2025

Deprecate TableDiff::getDroppedForeignKeys() #7157

Merged

morozov added the Schema Comparison label Sep 20, 2025

derrabus added this to the 4.4.0 milestone Oct 9, 2025


		## Deprecated dropping unnamed constraints in SQLite

		Dropping a constraint without specifying its name has been deprecated on the SQLite platform.

	foreach ($diff->getDroppedForeignKeys() as $foreignKey) {
	$sql[] = $this->getDropForeignKeySQL($foreignKey->getQuotedName($this), $tableNameSQL);
	}

		$constraintName = $droppedForeignKey->getObjectName()?->toString();
		if ($constraintName === null \|\| $constraintName === '') {


		# Upgrade to 4.4

		## Deprecated dropping unnamed constraints in SQLite

Uh oh!

Comments

Conversation

santysisi commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

santysisi commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Sep 10, 2025 • 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!

santysisi commented Sep 14, 2025

Uh oh!

santysisi commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Sep 16, 2025 • 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!

santysisi Sep 16, 2025 • 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!

morozov Sep 17, 2025 • 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!

Uh oh!

morozov commented Sep 17, 2025

Uh oh!

andersonamuller Sep 18, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

santysisi commented Sep 5, 2025 •

edited

Loading

morozov Sep 10, 2025 •

edited

Loading

morozov Sep 16, 2025 •

edited

Loading

santysisi Sep 16, 2025 •

edited

Loading

morozov Sep 17, 2025 •

edited

Loading

andersonamuller Sep 18, 2025 •

edited

Loading