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

Already on GitHub? Sign in to your account

2.1 - Added Missing PDO Driver Detection #593

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants

People in IRC always think the DB connection isn't working and are confused by this message. A surprising amount of people don't have the driver installed and have trouble, a new except w/clear messaging would be useful

I also tried to write a test using static mocks to mock PDO::getAvailableDrivers but it didn't work well:

http://bin.cakephp.org/view/1815762639

http://sebastian-bergmann.de/archives/883-Stubbing-and-Mocking-Static-Methods.html

dkullmann added some commits Mar 16, 2012

Added MissingDriverException for MySql to throw when it can't find th…
…e PDO driver. Otherwise DboSource throws "MissingConnectionException" and I spent 3 hours trying to figure out why I can login on the command line but CakePHP can't connect.
Use a different exception for MissingConnection and MissingDriver
See previous pull request from me. Utilizes a different error for MissingDriver when trying to load MySQL DB source

@markstory markstory and 1 other commented on an outdated diff Apr 2, 2012

lib/Cake/Error/exceptions.php
@@ -339,6 +339,17 @@ class MissingConnectionException extends CakeException {
+}
+
+ /**
+ * Used when a driver is missing
+ *
+ * @package Cake.Error
+ */
+class MissingDriverException extends CakeException {
@markstory

markstory Apr 2, 2012

Owner

Seems like this exception could be avoided by just re-using the MissingConnectionException.

@dkullmann

dkullmann Apr 2, 2012

Removed and used existing exception

@markstory markstory and 1 other commented on an outdated diff Apr 2, 2012

lib/Cake/Model/Datasource/Database/Mysql.php
@@ -174,6 +174,13 @@ public function connect() {
* @return boolean
*/
public function enabled() {
+ if(!$this->_PdoMysqlInstalled()) {

@markstory markstory commented on an outdated diff Apr 2, 2012

...ake/Test/Case/Model/Datasource/Database/MysqlTest.php
@@ -3487,6 +3487,22 @@ public function testExceptionOnBrokenConnection() {
'password' => 'inyurdatabase',
'database' => 'imaginary'
));
+
+ }
+
+/**
+ * @expectedException MissingDriverException
+ * @return void
+ */
+ public function testExceptionOnMissingDriver() {
+
@markstory

markstory Apr 2, 2012

Owner

No newline here.

dkullmann added some commits Apr 2, 2012

MissingDriverException removed in favor of reusing MissingConnectionE…
…xception

Also formatted the "if" statement properly
Removed MissingConnectionException in favor of PDOException
Unfortunately the ErrorRenderer doesn't work particularly nicely with strings, but, throwing a PDOException is an alternative that will display nice error message and ErrorRender is cool with.
Owner

markstory commented Apr 2, 2012

Do you think we should have similar errors for the other database drivers? Seems like the functionality could be generalized to fit all the existing adapters.

Sure - just throw the error from enabled()

David Kullmann

On Apr 1, 2012, at 11:19 PM, Mark Storyreply@reply.github.com wrote:

Do you think we should have similar errors for the other database drivers? Seems like the functionality could be generalized to fit all the existing adapters.


Reply to this email directly or view it on GitHub:
#593 (comment)

Member

ceeram commented Apr 2, 2012

Cant we just improve the error message in DboSource::__construct() ?

Owner

dkullmann commented on b4e7546 May 13, 2012

The only core handler that's affected is "DatabaseSession" and the problem is that doesn't properly calculate expiration, logging users out after the session is over.

CacheSession doesn't use __construct() so it's not affected

Ping

Owner

markstory commented Jun 12, 2012

I still don't like that we're only doing this for MySQL and all the other drivers are a bit out of luck. Also I don't understand why the session configuration was moved around.

Member

ceeram commented Jun 12, 2012

We could just throw the exception in DboSource::__construct() when $this->enabled() is false. It is indeed kinda confusing as both missing driver and unable to make pdo connection use same exception and message.

I think the session configuration shouldn't be part of the same pull request (github added it since I had two pull requests for the same branch)

David Kullmann

On Jun 12, 2012, at 2:58 PM, Mark Storyreply@reply.github.com wrote:

I still don't like that we're only doing this for MySQL and all the other drivers are a bit out of luck. Also I don't understand why the session configuration was moved around.


Reply to this email directly or view it on GitHub:
#593 (comment)

Owner

markstory commented Jun 13, 2012

@ceeram We could change the message for the MissingConnectionException that already exists so it is easier for people to figure out what is going on.

@ceeram @markstory

From today:

[17:35:12] cake 2.x works with PDO
[17:35:27] so make sure you have correct pdo_xxxxxx driver

I will paypal you $1 if you can make the missing PDO driver message more clear than it is. I don't really care if you use this pull request or not. Everytime someone is missing a PDO driver and thinks they can't connect to SQL for some other reason an angel loses it's wings. Or savant get's kicked out of a local bar... one of the two.

-DK

Member

ceeram commented Jun 14, 2012

I will get to it

@ghost ghost assigned ceeram Jun 14, 2012

Member

ceeram commented Jun 15, 2012

How about sth like this: a99f5394f478c976f89be5edc4e476d4fb2f1691

@ceeram I think that's great. I would recommend specifying that it's the PHP PDO driver if that's what's missing just to give a hint to t3h n00bs

Owner

markstory commented Jun 23, 2012

Lets get this [a99f539] merged in for 2.2.0 stable :D

YOU MUST BEAT CEREEM

David Kullmann

On Jun 22, 2012, at 8:56 PM, Mark Storyreply@reply.github.com wrote:

Lets get this [a99f539] merged in for 2.2.0 stable :D


Reply to this email directly or view it on GitHub:
#593 (comment)

Member

ceeram commented Jun 23, 2012

[a99f539] merged into 2.1

@ceeram ceeram closed this Jun 23, 2012

Owner

markstory commented Jun 23, 2012

Yay :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment