Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raising error on faulty save? #94

Closed
tomvo opened this issue Jan 15, 2013 · 9 comments
Closed

Raising error on faulty save? #94

tomvo opened this issue Jan 15, 2013 · 9 comments

Comments

@tomvo
Copy link

tomvo commented Jan 15, 2013

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
Copy link
Collaborator

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

@tomvo
Copy link
Author

tomvo commented Jan 15, 2013

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
Copy link
Collaborator

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 5af9da5 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
Copy link
Author

tomvo commented Jan 15, 2013

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
Copy link
Collaborator

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

@tag
Copy link
Contributor

tag commented Jan 15, 2013

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
Copy link
Collaborator

@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
Copy link
Author

tomvo commented Jan 15, 2013

@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
Copy link
Contributor

tag commented Jan 15, 2013

=)

Multiple connections are coming to Paris/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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants