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.

Sign up for GitHub

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

Jump to bottom

Fixes issue (DBAL-1057) use default platform when not connected #783

Closed
wants to merge 4 commits into from
Closed

Fixes issue (DBAL-1057) use default platform when not connected #783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23fc603
Fixes issue (DBAL-1057) where simply initiating the platform may caus…
weaverryan Jan 23, 2015
e90bdcd
Adding a test, which WAS failing before this patch
weaverryan Jan 23, 2015
8193333
Removing test that's failing, but because it is invalid - see #784
weaverryan Jan 23, 2015
2db254d
Changing implementation to *try* to connect
weaverryan Jan 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions lib/Doctrine/DBAL/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
namespace Doctrine\DBAL;

use Doctrine\DBAL\Driver\ServerInfoAwareConnection;
use Doctrine\DBAL\Exception\DriverException;
use Doctrine\DBAL\Exception\InvalidArgumentException;
use PDO;
use Closure;
Expand Down Expand Up @@ -424,9 +425,17 @@ private function getDatabasePlatformVersion()
return $this->_params['serverVersion'];
}

// If not connected, we need to connect now to determine the platform version.
// If not connected, we can't figure out the server version. If
// we try to connect, it may fail (e.g. no database), which would
// cause an error in our application when determining the platform
// version is not mission-critical
if (null === $this->_conn) {
$this->connect();
try {
$this->connect();
} catch (DriverException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still unhappy silencing all kind of connection errors as it also silences errors for valid credentials when there are other problems such as failing network or whatever.
Returning the default platform in such cases still might be fine 90% of the time but in IMO this is a hack to come around "invalid" (since 2.5) getDatabasePlatform() calls from userland. This really is breaking the feature that was implemented here and might be causing more trouble in the end than it solves.
The question is why do you need the platform at all if not connected? And what advantage do you have by retrieving just "some" platform that might be incompatible with your backend anyways? Before DBAL 2.5 that was just plain wrong IMO.
One might strongly disagree with my opinion here and "just don't care" but then all this platform version specific behaviour stuff that got implemented over the past few years is pretty useless.
I get the problems and the frustration this issue currently creates but we really please should not hack around that feature here. Instead we should fix calling libs to just use DBAL in the right way or revert the whole feature...
Just my 2 cents...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deeky666 the issue is that many places in Doctrine ORM (and in some places in DBAL too IIRC) are retrieving the platform eagerly in constructors to optimize performance by avoiding to call the platform getter multiple times.

// i.e. connection params are invalid or db doesn't exist
return null;
}
}

// Automatic platform version detection.
Expand Down
11 changes: 5 additions & 6 deletions tests/Doctrine/Tests/DBAL/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,13 +403,12 @@ public function testFetchColumn()
$this->assertSame($result, $conn->fetchColumn($statement, $params, $column, $types));
}

public function testConnectionIsClosed()
public function testGetPlatformVersionDoesNotFailOnBadConnection()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weaverryan why was this test actually removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is a valid test, which is what I'm arguing here: https://github.com/doctrine/dbal/pull/784/files#r23499338. I would like to remove this test - I was using that PR simply to abuse Travis to show how the test was passing for the wrong reason.

{
$this->_conn->close();

$this->setExpectedException('Doctrine\\DBAL\\Exception\\DriverException');

$this->_conn->quoteIdentifier('Bug');
// the connection uses bogus connection requirements, so connecting
// *will* fail. We should be able to call this without connection
// and causing that exception
$this->_conn->getDatabasePlatform();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing assertion (should assert that the returned value is null)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the database platform will never be null, and the point of the test is simply to show that asking for the database platform (with invalid connection credentials, which this test class uses) no longer causes an exception. If you removed my "fix" in this PR, calling getDatabasePlatform() would actually explode.

I can add an assertion that the database platform is in fact not null (it is always determine, it's just that it uses the default platform if it cannot connect to get the version), but the real point is that this doesn't cause an exception.

I feel like you and I aren't quite on the same page with this change and #784. I am vastly less qualified than you in this library, so perhaps it's my mistake, and if so - I appreciate the help clarifying :). Overall, I'm simply trying to fix the change in behavior where calling getDatabasePlatform() began throwing an exception if a connection couldn't be made (instead of using the default platform, like in earlier versions of DBAL).

Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

began throwing an exception if a connection couldn't be made (instead of using the default platform, like in earlier versions of DBAL).

Yeah, well, it's due to platform detection, as already discussed.

I'm also not very qualified to talk about the DBAL: it's mainly @deeky666's competence area.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I would like to hear what @deeky666 thinks then :).

I'm here because the exception has wide-reaching consequences that are affecting Symfony users - e.g. doctrine/DoctrineBundle#351 and symfony/symfony-standard#774.

Thanks for the attention!

}

public function testFetchAll()
Expand Down