fix(db): restore a foreign key its own rows violate

[HugoFara]

MigrationsTest::testForeignKeysSurviveDropAndRestore has been failing in full-suite runs while passing in isolation. It is not flaky — it was catching a real defect in the foreign-key repair, and hiding it at the same time.

The defect

addMissingForeignKeys() adds a constraint with a plain ALTER TABLE ... ADD CONSTRAINT, and InnoDB validates the existing rows when it does.

That contradicts the premise of the repair. The reason an install is missing a constraint is that it spent time without one, so the rows the constraint would have prevented are already in the database — reconcileForeignKeys()'s own docblock says exactly this:

Rows that a missing constraint would have prevented are already in the database. Adding under FOREIGN_KEY_CHECKS = 0 accepts them and gates writes from here on.

But nothing established that setting. update() and Restore::run() both happen to set it around their own migration runs, so the repair worked from there and nowhere else — the method inherited the condition from whichever caller was on the stack.

With checks on, InnoDB answers errno 1452 and refuses precisely the keys most worth restoring. The failure is caught and logged, so the constraint is silently lost.

Reproduced on the test schema

texttags rows: 3      text_tags rows: 0      orphans: 3

ALTER TABLE texttags ADD CONSTRAINT fk_texttags_text_tag ...
  FOREIGN_KEY_CHECKS = 1  ->  ERROR 1452 (23000): Cannot add or update a child row
  FOREIGN_KEY_CHECKS = 0  ->  added

Why it looked flaky

The test is self-concealing. A run that fails to restore the key leaves it dropped, so the next run's captureForeignKeys() "before" set does not contain it, nothing is expected back, and the test passes. It only fails again once something restores the key — a migration run in an earlier test, for instance. Pass and fail alternate on a schedule that has nothing to do with the code under test.

The fix

addMissingForeignKeys() sets FOREIGN_KEY_CHECKS = 0 for its own work and restores the previous value in a finally. The caller cannot know the setting is needed; it is a property of what the method does, not of when it is called. The add loop moves to addForeignKeysUnchecked() unchanged.

The two existing callers already set it, so their behaviour is identical.

Scope beyond the test

Restore::run() calls restoreForeignKeys() inside a finally that also sets checks back on — the ordering is correct today, but the dependency was invisible. Any future caller of reconcileForeignKeys() (admin repair tooling is the obvious one) would have silently dropped constraints on exactly the installs it was written to repair.

Checks

New test asserts the case directly, with a deliberately violating row; it fails without the fix (Failed asserting that an array contains 'texttags.fk_texttags_text_tag') and passes with it.

Psalm 0 errors · PHPCS clean · PHPUnit 9131 pass, 0 failures, across two consecutive full runs — the first time this suite has been green end to end in this branch's history. The texttags constraint is present afterwards with its 3 violating rows still in place, which is the intended repair semantics.