DBAL-990 Attempt platform detection even when the database name is not set #2671
Conversation
| // Either the platform does not support database-less connections | ||
| // or something else went wrong. | ||
| // Reset connection parameters and rethrow the original exception. | ||
| $this->_params['dbname'] = $databaseName; |
There was a problem hiding this comment.
Could this and the identical line below be put into a finally block?
There was a problem hiding this comment.
@mattjanssen I thought about that but as we need to backport this to 2.5 which still supports PHP 5.3, this unfortunately is not an option
There was a problem hiding this comment.
On this note, the merged fixes don't appear to have made it back to the 2.5 branch.
https://github.com/doctrine/dbal/blob/2.5/lib/Doctrine/DBAL/Connection.php#L405-L441
|
Just to check as I was reading this patch: does Doctrine cache the server version after having detected it? I ask because @Ocramius mentioned specifying it saves one query per request, as if it's not cached, which seems weird. |
|
@dkarlovi yes it kind of does because it is only required to create the correct platform instance which is then stored in the connection object. See here Also I am not quite sure what @Ocramius meant by saying "it saves one request" because we only retrieve the server version if the driver supports it without having to do another query. Most driver can provide it once connected without firing a query. In fact the only driver that does not is SQL Anywhere (currently) and you have to either specify the correct version during connection or it will fall back to a "default" platform. |
|
@dkarlovi no it is not cached. This is out of scope here. One could implement something like this in userland. But I really don't see why you should do that as platform detection as it is implemented does not cause any overhead. You will have to establish a new connection on a new request anyways, detecting the platform version is nothing more than simply connecting and retrieving a piece of information stored in the connection resource. What this PR deals with is edge cases where the platform is required even though the database to connect does not exist yet. |
|
@deeky666 sorry for spamming, I know it's outside the scope, just asked here as you seemed knowledgeable about the topic. Thanks for the answer. |
|
@dkarlovi no worries. Glad to clear that up :) Well, in the end I was the one that caused all the trouble because of implementing the feature :P |
|
@deeky666 branch name |
|
@dkarlovi hehe actually I was thinking about possible solutions for a long time but nothing reasonable came to my mind what we could do in DBAL. I think this one here is a fair compromise. |
| try { | ||
| $this->connect(); | ||
| } catch (\Exception $originalException) { | ||
| if (empty($this->_params['dbname'])) { |
There was a problem hiding this comment.
You could use ! $this->_params['dbname'].
There was a problem hiding this comment.
No I can't because it is possible to instantiate a connection object without this parameter. I even fell into this trap during writing the tests.
|
👍 This makes sense to me. Are we waiting for anything? This would close several issues, and help stop confusing users :). Looks like the last 1 comment from |
|
/cc @Ocramius mergie? |
lib/Doctrine/DBAL/Connection.php
Outdated
| return $this->getServerVersion(); | ||
| } | ||
|
|
||
| private function getServerVersion() |
There was a problem hiding this comment.
Documentation and return type for this function is missing
| $this->connect(); | ||
| try { | ||
| $this->connect(); | ||
| } catch (\Exception $originalException) { |
There was a problem hiding this comment.
Is there a more specific exception that we can catch here?
There was a problem hiding this comment.
@Ocramius I do not know what else we could catch here as the driver interface does not specify what to throw in case $driver->connect() fails. Any ideas welcome, though.
There was a problem hiding this comment.
That's a good-enough explanation for me
| $this->_conn->getEventManager() | ||
| ); | ||
|
|
||
| $this->assertInstanceOf('Doctrine\DBAL\Platforms\AbstractPlatform', $connection->getDatabasePlatform()); |
| */ | ||
| public function testDeterminesDatabasePlatformWhenConnectingToNonExistentDatabase() | ||
| { | ||
| if (in_array($this->_conn->getDatabasePlatform()->getName(), array('oracle', 'db2'), true)) { |
| /** @var \Doctrine\Tests\Mocks\VersionAwarePlatformDriverMock|\PHPUnit_Framework_MockObject_MockObject $driverMock */ | ||
| $driverMock = $this->createMock('Doctrine\Tests\Mocks\VersionAwarePlatformDriverMock'); | ||
|
|
||
| $connection = new Connection(array('dbname' => 'foo'), $driverMock); |
| public function testRethrowsOriginalExceptionOnDeterminingPlatformWhenConnectingToNonExistentDatabase() | ||
| { | ||
| /** @var \Doctrine\Tests\Mocks\VersionAwarePlatformDriverMock|\PHPUnit_Framework_MockObject_MockObject $driverMock */ | ||
| $driverMock = $this->createMock('Doctrine\Tests\Mocks\VersionAwarePlatformDriverMock'); |
|
Only big concern i have is the catch for \Exception else LGTM after Ocramius comments have been addressed. |
|
+1 for the comments from Ocramius. Hopefully we can catch a more specific exception... but even if we can't, the code is at least setup to throw that original exception eventually. /cc @deeky666 updatie? ;) |
deeky666
force-pushed
the
DBAL-990
branch
2 times, most recently
from
94e632a to
d458023
Compare
|
All green! 🚢 |
|
Unfortunately, this PR does not fix all issues. If username and password have not been set (for whatever reason), the code still throws an exception (unless anonymous database connections are allowed). IMHO, we should either return |
|
What is the use case for not having valid credentials being OK? The original issue was if you have valid credentials the system is able to create a database for you. That is unless it first tries to fetch the server's version, in which case it threw a "Missing Database" exception. Which is a silly exception if you're just asking the DBAL to create said database. But if you don't even have valid credentials, how would it create or query a database for you? |
|
We have a web-based GUI for creating the |
|
This was only merged to 2.6
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
…On Fri, Jun 30, 2017 at 12:40 PM, Chris Sedlmayr ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/Doctrine/DBAL/Connection.php
<#2671 (comment)>:
> + if (empty($this->_params['dbname'])) {
+ throw $originalException;
+ }
+
+ // The database to connect to might not yet exist.
+ // Retry detection without database name connection parameter.
+ $databaseName = $this->_params['dbname'];
+ $this->_params['dbname'] = null;
+
+ try {
+ $this->connect();
+ } catch (\Exception $fallbackException) {
+ // Either the platform does not support database-less connections
+ // or something else went wrong.
+ // Reset connection parameters and rethrow the original exception.
+ $this->_params['dbname'] = $databaseName;
On this note, the merged fixes don't appear to have made it back to the
2.5 branch.
https://github.com/doctrine/dbal/blob/2.5/lib/Doctrine/
DBAL/Connection.php#L405-L441
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2671 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJakL-_73-zWR1WwvuC4G2xKRlBOzsVks5sJNCwgaJpZM4MYeOX>
.
|
Ocramius
changed the title
|
@Ocramius: I’m not sure if this is entirely fixed in DBAL > 2.6 or if I have a different issue. I’m using DBAL+ORM with Symfony 2.8, my Doctrine packages are: When I run Apparently this has to do with ORM trying to determine the ID generator strategy for my entities. However, as soon as I set |
|
Don't think this issue is valid once taken out of the symfony cache
context: can you clean up the issue description to clarify what the problem
is, without including any symfony specifics?
…On 11 Sep 2017 16:48, "Alexander Günsche" ***@***.***> wrote:
@Ocramius <https://github.com/ocramius>: I’m not sure if this is entirely
fixed in DBAL > 2.6 or if I have a different issue.
I’m using DBAL+ORM with Symfony 2.8, my Doctrine packages are:
$ composer show | grep doctrine
doctrine/annotations v1.5.0 Docblock Annotations Parser
doctrine/cache v1.7.1 Caching library offering an object-oriented API for many cache backends
doctrine/collections v1.5.0 Collections Abstraction library
doctrine/common v2.8.1 Common Library for Doctrine projects
doctrine/dbal v2.6.2 Database Abstraction Layer
doctrine/doctrine-bundle 1.7.0 Symfony DoctrineBundle
doctrine/doctrine-cache-bundle 1.3.0 Symfony Bundle for Doctrine Cache
doctrine/inflector v1.2.0 Common String Manipulations with regard to casing and singular/plural rules.
doctrine/instantiator 1.0.5 A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer v1.0.1 Base library for a lexer that can be used in Top-Down, Recursive Descent Parsers.
doctrine/orm v2.5.10 Object-Relational-Mapper for PHP
When I run app/console cache:clear --no-optional-warmers --env=prod -v
with database_driver: pdo_mysql and server_version *not* being set, I get
the following exception trace:
[Doctrine\DBAL\Exception\ConnectionException]
An exception occurred in driver: SQLSTATE[HY000] [1045] Access denied for user ''@'localhost' (using password: NO)
Exception trace:
() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php:108
Doctrine\DBAL\Driver\AbstractMySQLDriver->convertException() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:176
Doctrine\DBAL\DBALException::wrapException() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:161
Doctrine\DBAL\DBALException::driverException() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOMySql/Driver.php:47
Doctrine\DBAL\Driver\PDOMySql\Driver->connect() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:372
Doctrine\DBAL\Connection->connect() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:432
Doctrine\DBAL\Connection->getDatabasePlatformVersion() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:396
Doctrine\DBAL\Connection->detectDatabasePlatform() at ./vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:338
Doctrine\DBAL\Connection->getDatabasePlatform() at ./vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:763
Doctrine\ORM\Mapping\ClassMetadataFactory->getTargetPlatform() at ./vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:616
Doctrine\ORM\Mapping\ClassMetadataFactory->completeIdGeneratorMapping() at ./vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:174
Doctrine\ORM\Mapping\ClassMetadataFactory->doLoadMetadata() at ./vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:333
Doctrine\Common\Persistence\Mapping\AbstractClassMetadataFactory->loadMetadata() at ./vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:78
Doctrine\ORM\Mapping\ClassMetadataFactory->loadMetadata() at ./vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:217
Doctrine\Common\Persistence\Mapping\AbstractClassMetadataFactory->getMetadataFor() at ./vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:115
Doctrine\Common\Persistence\Mapping\AbstractClassMetadataFactory->getAllMetadata() at ./vendor/symfony/symfony/src/Symfony/Bridge/Doctrine/CacheWarmer/ProxyCacheWarmer.php:69
Symfony\Bridge\Doctrine\CacheWarmer\ProxyCacheWarmer->warmUp() at ./vendor/symfony/symfony/src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php:48
Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate->warmUp() at ./vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:526
Symfony\Component\HttpKernel\Kernel->initializeContainer() at ./vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:133
Symfony\Component\HttpKernel\Kernel->boot() at ./vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php:134
Symfony\Bundle\FrameworkBundle\Command\CacheClearCommand->warmup() at ./vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php:96
Symfony\Bundle\FrameworkBundle\Command\CacheClearCommand->execute() at ./vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php:266
Symfony\Component\Console\Command\Command->run() at ./vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:893
Symfony\Component\Console\Application->doRunCommand() at ./vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:204
Symfony\Component\Console\Application->doRun() at ./vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:93
Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at ./vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:122
Symfony\Component\Console\Application->run() at ./app/console:29
Apparently this has to do with ORM trying to determine the ID generator
strategy for my entities.
However, as soon as I set server_version in my config, I can clear the
cache just fine.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2671 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJakD0G1xAEe-zIdlDePeEV5Y984R0Uks5shUg3gaJpZM4MYeOX>
.
|
|
You’re right, this has nothing to do with Symfony. I think the core of the problem is that |
Update Doctrine Bundle and ORM in order to use DBAL<2.6. Fix bug: doctrine/dbal#2671
|
I know this is supposedly fixed, but I still have trouble on DBAL 2.9.2 with |
|
Regarding @lxg's comment from above: What you're seeing is that once you need to access ORM metadata – be it because you ask the In your case (which I've run into as well) that is because the way the default Identifier Generation Strategy is chosen depends on the preferred mechanism for the platform. What this PR fixed is that the platform can be determined by connecting to the database without a particular schema being available. This allows to use DBAL to actually create the schema. Either way, it is still necessary that the database (server) can be connected to, and the username/password given/chosen need to at least allow connections. This also means that when you want to do things like warming caches (for example, when building a Docker image) or linting the Symfony Container, you need to have a database running or specify the |
|
adding |
Fixes #990
Fixes doctrine/DoctrineBundle#351
Closes #814
Closes #1109
and maybe more...
The long lasting problem with the connection not being lazy anymore and the database to connect to not existing (yet) will be fixed by this patch.
We now try to retrieve the platform version as usual. If connecting fails we try a fallback detection by unspecifying the database name and connecting again. If that works we fetch the platform version and disconnect again.
I think this extra connect operation is acceptable as it only occurs in case the database does not exist yet (edge case). So there should not be any noticeable performance impact.