MySQL getListTableForeignKeysSQL: use current database if null passed

[naderman]

In line with the behavior of getListTableIndexesSQL() the foreign key function should select only foreign keys from the current database if no database name is specified. Otherwise it returns foreign keys of all tables in any database with the given name. This can especially lead to issues if you install different versions of the same schema into multiple databases on the same server.

This function is always called with $database = null in the following chain, which leads to SQL errors when trying to setup/delete a schema in tests on a mysql server that contains another copy of the schema in another database:

Doctrine\ORM\Tools\SchemaTool::dropDatabase()
Doctrine\ORM\Tools\SchemaTool::getDropDatabaseSQL()
Doctrine\DBAL\Schema\AbstractSchemaManager::createSchema()
Doctrine\DBAL\Schema\AbstractSchemaManager::listTables()
Doctrine\DBAL\Schema\AbstractSchemaManager::listTableDetails($tableName)
Doctrine\DBAL\Schema\AbstractSchemaManager::listTableForeignKeys($table, null)
Doctrine\DBAL\Platforms\MySqlPlatform::getListTableForeignKeysSQL($table, null)

I think the $database parameter would ideally be required, and an exception should be thrown if it is null. The AbstractSchemaManager should be modified to consistently pass the database name to the platform in all its calls. But for now this workaround corrects the issue for foreign keys.

[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-1232

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

[beberlei]

@deeky666 This is a serious bug, because its very hard to find when it occurs to you.

[deeky666]

@naderman is it possible to add a functional test to avoid regressions in the future or do you think this is too much effort in this case?

[naderman]

@deeky666 Just haven't looked at doctrine's test set up at all so far, so no idea how much work this would be. Basically just need to create two databases with the same table with a foreign key each with different names and see if a select returns only the one on the right database.

[beberlei]

The tests still passing is a good indicator that it still works as expected in the scenario we care about (listTableDetails()). I couldn't think of a reliable way to test this with our testing setup without possibly breaking the testsuite. We do have two databases (doctrine_tests and doctrine_tests_tmp) but the later one has to be empty for some of the cleanup tricks we use.

[deeky666]

Can we try to avoid SQL duplication here by doing something like this:

$database = null === $database ? 'DATABASE()' : "'$database'";

[naderman]

@zeroedin-bill as we discussed on IRC this is rather hard to test in the current setup as it requires an additional database to be set up.

[naderman]

Did make the change @deeky666 suggested, so this should be mergable?

[naderman]

Test failure is a network issue on the travis box, you can restart those 2 failed builds and it should be fine.

[naderman]

@beberlei @deeky666 why was this not backported into the 2.5 branch? Kept wondering why this was still broken in 2.5.2 :(

[deeky666]

@naderman backport has been forgotten. Will tag this for 2.5.5 then.

[deeky666]

Backported into 2.5 via 43bb31e and e24444e

[naderman]

Thanks @deeky666 !