j4mie / idiorm

A lightweight nearly-zero-configuration object-relational mapper and fluent query builder for PHP5.
http://j4mie.github.com/idiormandparis/
2.01k stars 369 forks source link

Raising error on faulty save? #94

Closed tomvo closed 11 years ago

tomvo commented 11 years ago

guys, I noticed that Idiorm doesn't seem to raise an Exception on a faulty insert query during a save. This is according to the PDO spec which does also not raise an exception with a faulty query but instead sets the errorInfo property of the prepared statement.

I've been spending too much time over why my save call didn't actually save the record. I finally found the error when I used var_export($statement->errorInfo()); exit(); in the Idiorm save function.

What would be a solution to this so that it would become easier to spot the error of a faulty query on a save()?

treffynnon commented 11 years ago

Could you please include the code that was generating this error?

tomvo commented 11 years ago

It's not that Idiorm builds the query wrong, or does anything wrong for that matter, it only happens when you for example try to write a NULL value into a field that is not allowed to be NULL. PDO fails with a false and Idiorm just returns the result from the PDOStatement::execute without providing a way of finding out what the actual error was.

It's hard to include code that generates that, I can try to modify the tests to reproduce the problem but I'm not sure if that's going to work out.

treffynnon commented 11 years ago

Ah I see what you mean now. I am not sure if you have seen, but in the develop branch there is new code where you can actually get access to the last PDOStatement instance.

See https://github.com/j4mie/idiorm/commit/5af9da5e44abbf71558813dfd3a0e8de9b5f38f3 which is due out in the next release. The next release is due around the end of this month and the beginning of next month.

Does this patch help you enough or are you looking for something more robust like throwing an exception?

tomvo commented 11 years ago

I guess that patch does solve this problem a bit more elegantly/dynamic than just throwing an error. However I changed my fork to throw an error in case of a failure. See here

treffynnon commented 11 years ago

I agree and feel that accessing the PDOStatement seems more elegant so I am closing this issue.

tag commented 11 years ago

PDO should be throwing exceptions by default in Idriom (changed from the PDO default) ... see ORM::_setup_db(), where 'error_mode' is in ORM::_config as defaulting to PDO::ERRMODE_EXCEPTION:

    $db->setAttribute(PDO::ATTR_ERRMODE, self::$_config['error_mode']);

I'm concerned that forcing the check on PDOStatement->errorInfo() defeats the purpose of the ORM by requiring checks at a lower level, and further concerned, that as @tomvo points out, it is failing silently.

... Could it be @tomvo, that you're passing a PDO object to Idiorm directly, rather than letting it build one for you? Or am I just backward in expecting errors in PDOStatement would be thrown with this setting?

treffynnon commented 11 years ago

@tag You are definitely correct and it does trigger an exception.

For example here is one I have just triggered:

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[42S22]: Column not found: 1054 Unknown column 'test' in 'field list''

Perhaps @tomvo is sending in his own PDO instance, has changed the configuration to use a different ERRMODE or has display errors turned off in his PHP ini file etc.

tomvo commented 11 years ago

@tag What a smart catch, you are definitely right on suspecting i'm passing in a PDO object directly. I'm running a modified version of Idiorm that has support for multiple database connections. I'm passing in an array of PDO objects describing each database connection. I guess I missed out on the ATTR_ERRMODE attribute. I falsely assumed PDO always throws exceptions by default since it does so in the constructor. /cc @johnakkermans

tag commented 11 years ago

=)

Multiple connections are coming to Paris/Idiorm (j4mie/idiorm#15) although I'm behind on getting the fixes back to @treffynnon. It's coming soon. Really.

For posterity (should someone else find this issue in search), here are the docs showing enabling PDO exceptions, and a quick example of setting the PDO attribute if Idiorm doesn't build the PDO for you:

    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);