Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove handling orphaned foreign keys #5763

Merged
merged 1 commit into from
Oct 16, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Oct 15, 2022

The functionality being removed was deprecated in #5758.

@morozov morozov force-pushed the remove-orphaned-foreign-keys branch from 9d49104 to 0c08553 Compare October 15, 2022 21:41

$schemaDiff = $this->comparator->compareSchemas($oldSchema, $newSchema);

self::assertCount(1, $schemaDiff->changedTables['table_c']->getDroppedForeignKeys());
Copy link
Member Author

@morozov morozov Oct 15, 2022

Choose a reason for hiding this comment

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

This test is being removed as invalid. Consider instrumenting it with the following code:

$platform = new MySQLPlatform();

echo '-- Schema', PHP_EOL;
foreach ($oldSchema->toSql($platform) as $statement) {
    echo $statement, ';', PHP_EOL;
}

echo '-- Diff', PHP_EOL;
foreach ($schemaDiff->toSql($platform) as $statement) {
    echo $statement, ';', PHP_EOL;
}

If run on 4.0.x, it produces the following statements (the PRIMARY KEY clauses are not defined by the test schema and were added manually for the sake of correctness):

-- Schema
CREATE TABLE table_a (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_b (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_c (id INT NOT NULL, table_a_id INT NOT NULL, table_b_id INT NOT NULL, INDEX IDX_7365E39D71081393 (table_a_id), INDEX IDX_7365E39D63BDBC7D (table_b_id));
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D71081393 FOREIGN KEY (table_a_id) REFERENCES table_a (id);
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D63BDBC7D FOREIGN KEY (table_b_id) REFERENCES table_b (id);

-- Diff
ALTER TABLE table_a DROP FOREIGN KEY FK_7365E39D71081393;
DROP TABLE table_a;
ALTER TABLE table_c DROP FOREIGN KEY FK_7365E39D63BDBC7D;
DROP INDEX IDX_7365E39D71081393 ON table_c;
DROP INDEX IDX_7365E39D63BDBC7D ON table_c;
ALTER TABLE table_c DROP table_a_id, DROP table_b_id;

The assertion in question passes only because the DROP FOREIGN KEY FK_7365E39D71081393 statement is mistakenly executed on table_a although it should be executed on table_c.

With the functionality of handling orphaned foreign keys removed, the test starts failing because now the SQL looks like this:

-- Schema
CREATE TABLE table_a (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_b (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_c (id INT NOT NULL, table_a_id INT NOT NULL, table_b_id INT NOT NULL, INDEX IDX_7365E39D71081393 (table_a_id), INDEX IDX_7365E39D63BDBC7D (table_b_id));
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D71081393 FOREIGN KEY (table_a_id) REFERENCES table_a (id);
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D63BDBC7D FOREIGN KEY (table_b_id) REFERENCES table_b (id);

-- Diff
DROP TABLE table_a;
ALTER TABLE table_c DROP FOREIGN KEY FK_7365E39D71081393;
ALTER TABLE table_c DROP FOREIGN KEY FK_7365E39D63BDBC7D;
DROP INDEX IDX_7365E39D71081393 ON table_c;
DROP INDEX IDX_7365E39D63BDBC7D ON table_c;
ALTER TABLE table_c DROP table_a_id, DROP table_b_id;

Both constraints are dropped on table_c (which is correct) but this is not what the assertion expects.

Note that the resulting SQL is still invalid because table_a cannot be dropped while it's still referenced by a foreign key. The functionality of dropping multiple tables should be reworked accordingly (see the API introduced in #5416) but it seems to be out of the scope of this change since it didn't work properly even before this change.

Copy link
Member

Choose a reason for hiding this comment

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

A test that was passing by accident… must have been quite confusing, thanks for the detailed explanation!

@morozov morozov requested a review from greg0ire October 15, 2022 21:52
@morozov morozov marked this pull request as ready for review October 15, 2022 21:52
@morozov morozov requested a review from derrabus October 15, 2022 23:35
@morozov morozov merged commit 146ae85 into doctrine:4.0.x Oct 16, 2022
@morozov morozov deleted the remove-orphaned-foreign-keys branch October 16, 2022 16:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants