Deprecate overwriting existing foreign key constraints

[santysisi]

Q	A
Type	bug
Fixed issues	#7123

This PR introduces a deprecation notice when overwriting an existing foreign key constraint.

Summary

• Adds a deprecation warning when a foreign key constraint with the same name already exists.
• In Doctrine DBAL 5.0, this behavior will throw an exception instead of silently overwriting the constraint.

[santysisi]

status: needs work

I think it's better to move that logic into the _addForeignKeyConstraint method.

[santysisi]

status: needs review

I ended up adding the logic to both _addForeignKeyConstraint and addForeignKeyConstraint.

Why did I do this?
The _addForeignKeyConstraint method is called by both addForeignKeyConstraint and _construct. If I only modified addForeignKeyConstraint, it wouldn't impact the behavior in _construct.

So why modify addForeignKeyConstraint as well, if the logic already exists in _addForeignKeyConstraint?
That’s because addForeignKeyConstraint creates a ForeignKeyConstraint with a name. For the logic in _addForeignKeyConstraint to take effect, the ForeignKeyConstraint must not have a name set. But since addForeignKeyConstraint assigns one, the check in _addForeignKeyConstraint would never trigger.

One possible solution could be to remove the name generation in addForeignKeyConstraint, but I believe that would be a breaking change (BC).

Please let me know if this solution is incorrect or if any changes are needed.
Thanks again! 😄

[santysisi]

Maybe I'm wrong, but I think the pipeline errors aren't related to this PR

[santysisi]

should this PR be targeting the 4.3.x branch instead? 🤔

[morozov]

Maybe I'm wrong, but I think the pipeline errors aren't related to this PR

Yes, this is unrelated. Sorry about the inconvenience.

should this PR be targeting the 4.3.x branch instead? 🤔

That sounds right. You can change the base branch of this PR.

[santysisi]

Base branch changed to 4.3.x 😄

[santysisi]

Hey! 👋 I saw this PR and went ahead and updated the tests to use Table::editor() where appropriate.

[morozov]

@santysisi, thanks for the PR. Could you let us know whether it was prepared with the help of an AI tool, and if so, to what extent? This will helps us better understand how to approach reviewing it.

[santysisi]

Hi @morozov, thanks for your comment! Yes, I used an AI tool to help with writing comments (including PHPDoc) and to suggest some method and variable names. This comment is also written with the help of AI, as I'm still working on improving my English, and these tools help me communicate more clearly.

Please let me know if there's anything you'd like me to revise. I'm happy to make changes!

[santysisi]

Hi @morozov, thanks a lot for taking the time to review the PR, I really appreciate it! 😊

If I understood correctly, the idea now is to trigger a deprecation notice only in case of a collision, but still continue with the current behavior (i.e., overwrite).
Could you please confirm that? 🙏

If so, I can update the PR this weekend to reflect that behavior.

Thanks again! ❤️

[morozov]

the idea now is to trigger a deprecation notice only in case of a collision, but still continue with the current behavior (i.e., overwrite).

Correct. This will become an exception in 5.0.x.

[santysisi]

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.

[morozov]

We're getting close. Besides a few more comments, looks good.

[santysisi]

Thanks a lot for your suggestions, the changes have been made

[derrabus]

What should I do instead?

[morozov]

Good question. In order to replace an existing constraint, drop it first via dropForeignKey().

@santysisi please add this to the upgrade notes.

[morozov]

Thanks, @santysisi. Congrats on your first contribution!

[santysisi]

Thanks for your help! 😄

[morozov]

@santysisi, I merged the changes from this and your other PR up into 5.0.x. If you're up for it, you can convert these deprecations to exceptions.

[santysisi]

Yep! 😄
I'll be opening those PRs today ❤️

Edit: I don't have enough time today 😔, I will try to do it this weekend

[santysisi]

Hi @morozov,

While working on the new PR (to replace the deprecation with throw and error), I encountered an issue (reproducible with SQLite — I’ve tested with SQLite and PostgreSQL only so far) in this method getForeignKeysInAlteredTable.

The problem stems from this change: #7116, specifically this part:
https://github.com/doctrine/dbal/blob/5.0.x/src/Schema/Table.php#L707-L710

Previously, we used unset($foreignKeys[$constraintName]), but since the method now returns a list, unsetting by key doesn’t work anymore.

Before proceeding with the final deprecation removal and introducing the new error, I think we should address this issue first.

I’d be happy to take a shot at fixing it! 😄

I see two possible approaches:

Adjust the existing method:
Keep the current method that returns a list, but calculate the index of the constraint in the list and unset() by position. Alternatively, rebuild the array to use the constraint names as keys.

Introduce a new method:
Add a separate method that returns the foreign keys as an associative array (like before), without modifying the current list-based method.

Let me know what you think!

[santysisi]

Just as an example — if we don’t want to introduce a new method that returns an associative array with constraint names as keys, we could handle it like this:

$foreignKeys = [];
foreach ($oldTable->getForeignKeys() as $key => $foreignKey) {
    $constraintName = $foreignKey->getObjectName();
    
    if ($constraintName === null) {
        $foreignKeys[$key] = $foreignKey;
        continue;
    }

    $foreignKeys[strtolower($constraintName->getIdentifier()->getValue())] = $foreignKey;
}

This way, we preserve the ability to reference foreign keys by their constraint names (like before), while still using the current method that returns a list.

It’s a simple alternative to avoid breaking changes or adding a new method, instead of just doing:

$foreignKeys = $oldTable->getForeignKeys();

Another alternative could be a deeper refactor 🤔

[morozov]

Previously, we used unset($foreignKeys[$constraintName]), but since the method now returns a list, unsetting by key doesn’t work anymore.

Yeah, I noticed this issue as well. Indexing by name makes sense. The only thing is that your code still mixes name-based and positional keys, which is prone to conflicts (0 could be both a position of an unnamed constraint and a name of a named one).

Instead, you could introduce and populate a map of constraint names to their numeric keys in the $foreignKeys array (e.g. $keysByName) – only for those constraints that have a name. Then, while processing dropped FKs:

1. Assert that it has a name
2. Check if there's an entry in $keysByName for its name
3. If there is, unset the corresponding key in $foreignKeys