Check for foreign table name on removed tables foreign key

[mpoiriert]

According to the comment

// deleting duplicated foreign keys present on both on the orphanedForeignKey
// and the removedForeignKeys from changedTables

A check must be had on the $removedForeignKey so it does point on the removed table. Currently it does unset all keys removal even the one pointing on other table.

This should probably be added to a previous version since it's a bug fix but I don't know the exact flow you are following for this.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1236

We use Jira to track the state of pull requests and the versions they got
included in.

[deeky666]

Requires a test case.

[deeky666]

@mpoiriert ping. Please provide a test case or at least give information on how to reproduce the actual issue. Unfortunately the code and your description don't give enough info to reproduce.

[mpoiriert]

I know we must use entities but I'll use table to explain the problem.

The loops is meant to be to remove duplicate removal of foreign key: one from the drop table and one from the foreign key being removed from the other table in the relation.

To replicate the problem create 3 tables:

table_a, table_b and table_c.

table_3 have 2 foreign keys, one pointing on table_a and another one pointing on table_b:

CREATE TABLE table_a (
    id INT NOT NULL,
    PRIMARY KEY (id)
) ENGINE=INNODB;

CREATE TABLE table_b (
    id INT NOT NULL,
    PRIMARY KEY (id)
) ENGINE=INNODB;

CREATE TABLE table_c (
    id INT, 
    table_a_id INT,
    table_b_id INT,
    INDEX table_a_id (table_a_id),
    INDEX table_b_id (table_b_id),
    FOREIGN KEY (table_a_id_fk) 
        REFERENCES table_a (id)
        ON DELETE CASCADE,
    FOREIGN KEY (table_b_id_fk) 
        REFERENCES table_b (id)
        ON DELETE CASCADE
) ENGINE=INNODB;

Once the entites/table are created remove the entity using table_a and the proper foreign key from table_c. Also remove the foreign key from table_c that point on table_b but keep table_b. You should end up with this as a final result:

CREATE TABLE table_b (
    id INT NOT NULL,
    PRIMARY KEY (id)
) ENGINE=INNODB;

CREATE TABLE table_c (
    id INT, 
    table_b_id INT
) ENGINE=INNODB;

Without my patch we end up with this:

CREATE TABLE table_b (
    id INT NOT NULL,
    PRIMARY KEY (id)
) ENGINE=INNODB;

CREATE TABLE table_c (
    id INT, 
    table_b_id INT,
    INDEX table_b_id (table_b_id),
    FOREIGN KEY (table_b_id_fk) 
        REFERENCES table_b (id)
        ON DELETE CASCADE
) ENGINE=INNODB;

This is because all the foreign key drop of table_c have been removed in the loop since no check on the key is done related on which table it's pointing on. So drop of foreign key table_b_id_fk is remove from the list when it should not since it's pointing on table_b that is still existing.

Is it clear enough ?

[deeky666]

@mpoiriert thanks for your feedback. I think I can work with that for now. Will check that and poke you again if anything remains unclear :)

[deeky666]

@mpoiriert sorry can't reproduce the issue if I run the following test case:

$oldSchema = new Schema();

$tableA = $oldSchema->createTable('table_a');
$tableA->addColumn('id', 'integer');

$tableB = $oldSchema->createTable('table_b');
$tableB->addColumn('id', 'integer');

$tableC = $oldSchema->createTable('table_c');
$tableC->addColumn('id', 'integer');
$tableC->addColumn('table_a_id', 'integer');
$tableC->addColumn('table_b_id', 'integer');

$tableC->addForeignKeyConstraint($tableA, array('table_a_id'), array('id'));
$tableC->addForeignKeyConstraint($tableA, array('table_b_id'), array('id'));

$newSchema = new Schema();

$tableB = $newSchema->createTable('table_b');
$tableB->addColumn('id', 'integer');

$tableC = $newSchema->createTable('table_c');
$tableC->addColumn('id', 'integer');

$comparator = new Comparator();
$schemaDiff = $comparator->compare($oldSchema, $newSchema);

$this->assertCount(0, $schemaDiff->changedTables['table_c']->removedForeignKeys);
$this->assertCount(2, $schemaDiff->orphanedForeignKeys);

It works with and without your patch. At least your patch should make a difference but apparently it doesn't. What am I missing here?

[deeky666]

@mpoiriert sry the test is borked. Will reinvestigate.

[mpoiriert]

You must keep the table_c table_b_id column but remove the foreign key.

If you are able to make the test (even it does pass without my patch) I'll manage from there, I wasn't sure the best way to test that...

Also is it possible that something else have fix the issue since my fix ? Didn't check the commit history...

[deeky666]

@mpoiriert I found the error in my test. Now it reveals the issue and your patch fixes it. I'll add the test and then merge manually. Thanks!

[mpoiriert]

Glad to help, it was a edge case but when it happen a while ago I scratch my head a lot !!!

[deeky666]

Merged manually via f5f99cd adding a covering test case and fixing trivial CS issues. Thanks @mpoiriert

[deeky666]

Backported to 2.5 branch via ed7e49b, c2f626f, 51a0529