fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Migration swallows access denied, throws invalid DSN #1264

Closed Altreus closed 11 years ago

Altreus commented 11 years ago

When running a migration, the PDO exception for access denied is swallowed, and then as a result throws the invalid DSN exception.

I traced this to fuel/core/classes/migrate.php:551

if ( ! \DBUtil::table_exists(static::$table))

This function catches the Database_Exception thrown by PDO that says access denied. This leaves the Db class with no connection.

    \DBUtil::create_table(static::$table, static::$table_definition);

This then causes the Db class to try to connect again. Then the Db class treads on its own feet, at fuel/core/classes/database/pdo/connection.php:65

$this->_config['connection'] = array();

So the next time it tries to connect, it has invalidated its own DSN. Now, the PDOException is thrown, but it is not caught by DBUtil::create_table - this function simply allows it to be thrown, whereupon it reaches frame 0 and kills the migration, informing the user that the DSN is wrong, when in fact the Db class made it wrong, then complained.

It is certainly incorrect to use the exception to test whether the table exists - exceptions are for exceptional behaviour, not functional behaviour. I'm sure there must be a way of asking MySQL whether a table exists, and even if there is not this catch handler needs to be much more specific about when it is going to ignore the exception.

WanWizard commented 11 years ago

The problem with that is that since FuelPHP's DB access is driver based, you don't have any clue as to which RDBMS is actually used. So you need a way of detecting the existance of a table without resorting to platform specific code.

With the new query builder that is going to be introduced in 2.0, the util code has been moved into the drivers, which solves that issue, but that doesn't help this case.

If you know a better way of detecting the tables existance, you're welcome to share it.

WanWizard commented 11 years ago

Does this workaround fix it: https://github.com/fuel/core/commit/3101aa9db230b98d056a8914f72ab466423358f2 ?

WanWizard commented 11 years ago

@Altreus any feedback?

Altreus commented 11 years ago

Sorry, completely overlooked this. I'll set up a test to try it when I've got some more slack time available.

WanWizard commented 11 years ago

Thanks!

Altreus commented 11 years ago

This is OK on the web, but not in migrations, for me.

I think the issue lies in whether you're using DB or DBUtil - my stacktrace for when it doesn't work goes through database/query.php, but dbutil.php still gives me the old issue.