Deprecate TableDiff::getDroppedForeignKeys()#7157
Conversation
| } | ||
|
|
||
| /** @return array<UnqualifiedName> */ | ||
| public function getDroppedForeignKeyConstraintNames(): array |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
37e9bf9 to
729b0a1
Compare
The problem
Currently,
TableDiffcontains a list of dropped foreign key constraints. This is wrong for two reasons:This way, we get what we don't need and we don't get what we need.
Manifestations in the code
dbal/src/Platforms/AbstractPlatform.php
Lines 1372 to 1374 in e2e36dd
dbal/src/Platforms/SQLitePlatform.php
Lines 933 to 938 in 6aa4070
dbal/src/Schema/SQLiteSchemaManager.php
Lines 52 to 59 in 6aa4070
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:
$droppedForeignKeysofTableDiff#7143).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.