Skip to content

Comments

Deprecate TableDiff::getDroppedForeignKeys()#7157

Merged
morozov merged 1 commit intodoctrine:4.4.xfrom
morozov:deprecate-dropped-foreign-key-constraints
Sep 25, 2025
Merged

Deprecate TableDiff::getDroppedForeignKeys()#7157
morozov merged 1 commit intodoctrine:4.4.xfrom
morozov:deprecate-dropped-foreign-key-constraints

Conversation

@morozov
Copy link
Member

@morozov morozov commented Sep 20, 2025

The problem

Currently, TableDiff contains a list of dropped foreign key constraints. This is wrong for two reasons:

  1. Dropping a constraint doesn't require knowing its full definition. The name is sufficient.
  2. The name is also necessary, but constraint names are optional, so the definition isn't guaranteed to contain a name.

This way, we get what we don't need and we don't get what we need.

Manifestations in the code

  1. In some cases, unnamed dropped foreign key constraints will lead to invalid SQL:
    foreach ($diff->getDroppedForeignKeys() as $foreignKey) {
    $sql[] = $this->getDropForeignKeySQL($foreignKey->getQuotedName($this), $tableNameSQL);
    }
  2. In some cases, they will be silently ignored:
    foreach ($diff->getDroppedForeignKeys() as $constraint) {
    $constraintName = $constraint->getName();
    if ($constraintName === '') {
    continue;
    }
  3. In another case, this design requires a weird back-and-forth translation in order to satisfy the API:
    public function dropForeignKey(string $name, string $table): void
    {
    $table = $this->introspectTable($table);
    $foreignKey = $table->getForeignKey($name);
    $this->alterTable(new TableDiff($table, droppedForeignKeys: [$foreignKey]));
    }
    The foreign key constraint definition is extracted from the table definition and is then passed alongside the table definition itself only to satisfy the API. Subsequently, the definition will be discarded, and only the name will be used.

Proposed solution

Instead of containing a list of dropped foreign key constraints, the diff should contain a list of their names.

Scope of the change

We cannot modify production code, because:

  1. The constraints being dropped aren't guaranteed to have a name (see Deprecate unnamed foreign key constraints in $droppedForeignKeys of TableDiff #7143).
  2. The names themselves aren't guaranteed to be valid UnqualifiedNames until 5.0.

The test is modified to demonstrate the proper use of the API.

Future scope

Likely, the same logic applies to all other dropped objects in the diff. I'd prefer to rework this logic object by object in order to minimize the diffs.

}

/** @return array<UnqualifiedName> */
public function getDroppedForeignKeyConstraintNames(): array
Copy link
Contributor

@santysisi santysisi Sep 20, 2025

Choose a reason for hiding this comment

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

Maybe we could create a private property (not readonly and not set via the constructor, with a default value of []) called something like droppedForeignKeysConstraintNames. Then we could execute this foreach loop only once, store the result in that property, and on subsequent calls, just return the cached array if it's already populated.

That said, this might be overengineering, especially if this tableDiff method isn't expected to be called frequently or if it doesn't usually have many dropped foreign keys. 🤔

What do you think 😄 ?

🤔 After looking at the code again, I’m not so sure this approach is a good idea. Might introduce unnecessary complexity for little gain.

Copy link
Member Author

@morozov morozov Sep 20, 2025

Choose a reason for hiding this comment

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

The main concern is not the complexity. A call to ForeignKeyConstraint::getObjectName() in 4.4.x may throw an exception. If it's thrown from the TableDiff constructor, it would be a BC break. If it's thrown from the new method, it's fine because users will have to opt into calling it.

If we take the approach with caching, we'd have to cache not only the names but also the exception. This is indeed overkill.

@morozov morozov requested a review from greg0ire September 24, 2025 19:15
greg0ire
greg0ire previously approved these changes Sep 25, 2025
@morozov morozov force-pushed the deprecate-dropped-foreign-key-constraints branch from 37e9bf9 to 729b0a1 Compare September 25, 2025 05:23
@morozov morozov merged commit 15c1c0a into doctrine:4.4.x Sep 25, 2025
96 checks passed
@morozov morozov deleted the deprecate-dropped-foreign-key-constraints branch September 25, 2025 05:38
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.

3 participants