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

weaverryan · 2015-01-23T17:37:13Z

Hi guys!

This is a fix for http://www.doctrine-project.org/jira/browse/DBAL-1057. It leaves the ability to set the platform and platform version from sha: 3176f51, but does not try to connect if we're not already connected. This is consistent with the previous behavior (again see the linked sha) - before there was never a connection made to determine the platform.

The alternate solution is to connect, but surround this by a try-catch (PDOException, DriverException, Exception?) and return null. in case we have some situation where the database isn't created or the connection information is wrong.

I know this issue is causing a lot of problems around my world (I ran into myself yesterday), so thanks in advance for the attention.

Thanks!

…e an error For example, if you have no database, then previously, we would try to connect, causing an error. But in this case, we can simple fall back to the default platform. This is exactly what the previous version did (see sha: 3176f51), and thus this adds the nice fallback we had before, without removing the flexibility of setting your exact server version or determining it once there *is* a connection

doctrinebot · 2015-01-23T17:37:26Z

Hello,

thank you for creating this pull request. However did not open it on the "master"
branch. Our Git workflow requires all pull requests to go through "master" branch
and the release masters then merge them back into stable branches, if they are
bug fixes.

Please open the pull request again for the "master" branch and close
this one.

Nevertheless I have opened a Jira ticket for this Pull Request to track this
issue:

http://www.doctrine-project.org/jira/browse/DBAL-1129

We use Jira to track the state of pull requests and the versions they got
included in.

Ocramius · 2015-01-23T17:38:07Z

@weaverryan can you inspect testability of this change?

weaverryan · 2015-01-23T17:38:41Z

@Ocramius yep, let me see about that now. Is this ok that it's against the 2.5 branch? My branch is based off of 2.5.

Ocramius · 2015-01-23T17:39:15Z

@weaverryan we can handle merging back int master, yeah: we'd simply cherry pick and rebase

…e#784

weaverryan · 2015-01-23T18:19:50Z

@Ocramius I've just added a test for this, it's very simple: calling Connection::getDatabasePlatform() in ConnectionTest WAS failing before this patch, because the params passed to Connection in the test are bogus. The fact that we can call this (with bogus params) and not get an exception shows the functionality.

I also removed another test that I think os bogus - see #784.

Thanks!

For me, this makes more sense. Without this, if getDatabasePlatform() is called consistently before connection(), then the platform will *never* connect to get the version and use the *best* platform - it will always use the fallback. This *does* use the connection, but doesn't explode for the edge cases when the connection information is invalid.

weaverryan · 2015-01-23T18:32:00Z

I just changed the implementation to use try-catch at sha: 2db254d. I can go either way but this makes more sense to me.

Thanks!

Ocramius · 2015-01-24T08:26:29Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

@@ -403,13 +403,12 @@ public function testFetchColumn()
        $this->assertSame($result, $conn->fetchColumn($statement, $params, $column, $types));
    }

-    public function testConnectionIsClosed()


@weaverryan why was this test actually removed?

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.

SoboLAN · 2015-01-26T14:42:21Z

I think surrounding the connection statement with try-catch and returning null if it fails is more of a hack than a fix.

What I mean is... let's have an example:

if I have 2 projects in Symfony: project A and project B
project A depends on Doctrine/DBAL, project B does not

In this situation, if I only use project B, then there will always be a connection made to the database, even if I never use it. Not really ideal.

stof · 2015-01-26T14:44:01Z

@SoboLAN if you have 2 projects, you should probably use 2 kernels, so that your project which does not need DBAL does not load it at all. It is not related to this fix at all

weaverryan · 2015-01-27T00:30:54Z

Hi guys!

I just hit this in a room of trainees today as we started a new Symfony 2.6 project. We tried to generate an entity (not a DB-connecting operation), and it bombed out (we hadn't even talked DB credentials yet). This seems small, but in practical terms, it affects all new Symfony 2.6 projects the moment they first interact with Doctrine. I'm happy to make any changes necessary.

@SoboLAN I won't agree or disagree actually :). But regardless, a connection is made currently if you ask for the database platform - that's a fact of this DBAL version (I want to not discuss it here for reasons of focus). This PR simply says: if that existing "open a connection" fails while figuring out the DB platform, don't bomb out, just use the default platform (which is the behavior of the last DBAL version).

Thanks for everyone's time :)

Ocramius · 2015-01-27T04:43:49Z

OT: please don't teach entity generation to trainees ;-)

deeky666 · 2015-01-27T08:21:45Z

lib/Doctrine/DBAL/Connection.php

-            $this->connect();
+            try {
+                $this->connect();
+            } catch (DriverException $e) {


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...

@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.

weaverryan · 2015-01-29T00:37:42Z

@Ocramius but if I make them do it by hand the first time, they will kick me out of the room :p

weaverryan · 2015-01-29T01:28:14Z

@deeky666 Thanks very much for the clarification! I don't love the BC break, but I also see that it's well-documented in the UPGRADE log :).

I have opened doctrine/orm#1294, which i believe fixes the biggest "userland" bad use of this method (indeed, it's calling getDatabasePlatform() in situations where it doesn't even need that information).

Thanks!

deeky666 · 2015-01-29T09:37:18Z

@weaverryan thank you for your patience and help! Fixing the ORM to avoid unneccesary getDatabasePaltform() calls is a good first step. I believe the bigger problem (Symfony-wise) is in the DoctrineBundle here. Registering mapping types requires retrieving the platform currently. We have to find a solution for that so that commands like doctrine:database:create work again. I believe the ConnectionFactory is the biggest problem as it is utilized nearly everywhere in Symfony userland AFAIK.

stof · 2015-01-30T02:30:15Z

@deeky666 for DoctrineBundle, anyone creating DBAL types with the uptodate ways to mark them as commented or as mapped to DB types (i.e. in the type itself) would not trigger this code anymore (it is the legacy way to register the type mapping). Thus newcomers won't be registering custom types. This is not the cause of their issue.
The bigger issue for Symfony users is because of the issues in the ORM.

weaverryan closed this Jan 23, 2015

weaverryan reopened this Jan 23, 2015

weaverryan added 2 commits January 23, 2015 13:15

Adding a test, which WAS failing before this patch

e90bdcd

Removing test that's failing, but because it is invalid - see doctrin…

8193333

…e#784

weaverryan mentioned this pull request Jan 23, 2015

Prevent doctrine/dbal 2.5 from installing symfony/symfony-standard#774

Merged

Ocramius reviewed Jan 24, 2015
View reviewed changes

Ocramius mentioned this pull request Jan 24, 2015

Connection is not lazy anymore, since platform detection was introduced - exposing the issue via broken test #784

Merged

deeky666 reviewed Jan 27, 2015
View reviewed changes

weaverryan closed this Jan 29, 2015

This was referenced Dec 6, 2015

DBAL-1058: It seems that MSSQL syntax was changed #991

Closed

DBAL-1129: [GH-783] Fixes issue (DBAL-1057) use default platform when not connected #1070

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

weaverryan commented Jan 23, 2015

doctrinebot commented Jan 23, 2015

Ocramius commented Jan 23, 2015

weaverryan commented Jan 23, 2015

Ocramius commented Jan 23, 2015

weaverryan commented Jan 23, 2015

weaverryan commented Jan 23, 2015

Ocramius Jan 24, 2015

weaverryan Jan 24, 2015

SoboLAN commented Jan 26, 2015

stof commented Jan 26, 2015

weaverryan commented Jan 27, 2015

Ocramius commented Jan 27, 2015

deeky666 Jan 27, 2015

stof Jan 30, 2015

weaverryan commented Jan 29, 2015

weaverryan commented Jan 29, 2015

deeky666 commented Jan 29, 2015

stof commented Jan 30, 2015

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

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

Conversation

weaverryan commented Jan 23, 2015

doctrinebot commented Jan 23, 2015

Ocramius commented Jan 23, 2015

weaverryan commented Jan 23, 2015

Ocramius commented Jan 23, 2015

weaverryan commented Jan 23, 2015

weaverryan commented Jan 23, 2015

Ocramius Jan 24, 2015

Choose a reason for hiding this comment

weaverryan Jan 24, 2015

Choose a reason for hiding this comment

SoboLAN commented Jan 26, 2015

stof commented Jan 26, 2015

weaverryan commented Jan 27, 2015

Ocramius commented Jan 27, 2015

deeky666 Jan 27, 2015

Choose a reason for hiding this comment

stof Jan 30, 2015

Choose a reason for hiding this comment

weaverryan commented Jan 29, 2015

weaverryan commented Jan 29, 2015

deeky666 commented Jan 29, 2015

stof commented Jan 30, 2015