Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

PDO connection exceptions are ignored #371

Closed
masom opened this Issue · 20 comments

6 participants

@masom

data\source\database\adapter\MySql.php

In the connection method, any error raised by the PDO adapter is ignored and false is returned.

Not very helpful if someone mistyped their password.

The PostgreSQL pdo adapter raises things like:
string(73) "SQLSTATE[08006] [7] FATAL: password authentication failed for user "li3""

@nateabele
Owner

What's the actual exception thrown, just a generic PDOException?

@marcghorayeb

No exception is thrown, false is returned to the database adapter so it thinks it's connected and will usually try to call the quote() method on it. Since the object is not instantiated, it will throw a fatal error.

I previously didn't open a bug report because i had seen it fixed in one of the branches but after double checking the fix seems to have disappeared. I usually do throw $e; on line 143 instead of the return false in MySql.php.

@daschl
Collaborator

Jup, the problem is that the PDOExceptions are caught on connect, but only false is returned. Personally, I think we should either raise a standardized exception across all data sources there (ConnectError, you name it) or just let it bubble up so that the error/exception catching mechanisms can deal with it.

@nateabele ?

@nateabele
Owner

@marcghorayeb Yes, I know. I was asking which exception PDO itself throws.

@daschl Agreed. What I care about is which exception to throw. For example, the Mongo adapter throws a NetworkException. In MySQL's case, it could either be a ConfigException (the host exists but the database name is invalid or your credentials are wrong) or a NetworkException (can't connect to the host at all).

Does anyone know if PDO provides varying error codes that we can check to determine which exception to throw?

@masom

It seems PDO will provide different error codes depending on the error and the backend. We might be able to determine what kind of exception to re-raise (ConfigException, NetworkException) depending on the error string.

@nateabele
Owner

Awesome. I just took another look through MySql::connect(), and with a with a little simple refactoring, we can move that up to Database.

@jrgns

This is still an issue. More information on the error codes (retrieved by doing $e->getcode()) here and the official docs.

There's a whole bunch of codes, not sure how you guys want to handle this. I'm willing to do the coding, I'm just not sure which codes map to which exceptions.

@nateabele
Owner

@jrgns Cool, thanks for the offer.

Basically, all database issues break down into one of three exceptions: NetworkException and ConfigException, which are both in lithium\core, and QueryException, which is in lithium\data\model.

Anything affecting the app's ability to connect to the host (CR_UNKNOWN_HOST or CR_CONN_HOST_ERROR) is a NetworkException. Anything indicating a bad configuration (ER_ACCESS_DENIED_ERROR, ER_BAD_DB_ERROR, or anything else indicating that the host connection is good, but it can't talk to the database), that's a ConfigException.

QueryException is for basically anything that goes horribly wrong while trying to execute a query.

Those first two are gonna be probably the only ones you need for connect(), and as far as how to map all possible error codes, just use your best judgement, and ask if you have any questions.

Thanks, dude!

@jrgns

Kewl, I've added some prelim code (jrgns@94ba5c9). I'll test and refine later.

@jrgns

Second try: jrgns@36fb107

@daschl
Collaborator

@jrgns I think we need to port sqlite first over to pdo if we refactor it one level up, right?

@jrgns

@daschl Only necessary if you want SQLite on PDO. As I see it, Database now provides basic PDO functionality which can be used (like the MySQL adapter) or not (like the SQLite adapter)

Do you want to use PDO for SQLite?

@jails
Collaborator

Well i think we can forget SQLite for the moment The overriding of the connect() methods here will continue to do its job here for SQLite.

One or two typos to fix but the patch looks fine for me. If you need help for the tests this can help you https://gist.github.com/2787357, it will manage the main exceptions. (to put in lithium\tests\integration\data\DatabaseTest.php imo)

@jrgns

@jails Shot, thanx.

@nateabele
Owner

Here you go: https://gist.github.com/2788016

I re-did it slightly against the data branch, since that's where it'll be committed to. Totally untested and might even have syntax errors, but otherwise it should be good. :-)

I think I know of someone who might be able to update the SQLite adapter to PDO. I'll ask.

@jails
Collaborator

@jrgns need some futher help ?

@jrgns

@jails Nope, thanx. Just incorporated the changes @nateabele suggested. I'll be adding tests later. Committed to jrgns@6a7a5ae

@jails
Collaborator

Cool I updated integration tests according to @nateabele changes. https://gist.github.com/2787357

@jails
Collaborator

Just notice that the "denied access error" would be managed by : in_array($code, array('28000', '42000')) and not in_array($code, array('28000', '4200')). Otherwise it looks good ;-)

@nateabele
Owner

These changes have been integrated into the data branch, and will be merged to master shortly.

@nateabele nateabele closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.