create:database fails, when dbname is set via URL

[kingcrunch]

First of all: This may be a DBAL-issue.

However, when I specify the database connection via URL like mysql://foo:bar@localhost/dbname the command doctrine:data:create fails with an exception. I was able to extract the original exception from the driver (see below). It works, when I omit dbname from the URL and set it directly with the dbname configuration key.

#0 /vagrant/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOMySql/Driver.php(45): Doctrine\DBAL\Driver\PDOConnection::__construct('mysql:host=127....', 'root', 'root', Array)
#1 /vagrant/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(360): Doctrine\DBAL\Driver\PDOMySql\Driver->connect(Array, 'root', 'root', Array)
#2 /vagrant/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php(973): Doctrine\DBAL\Connection->connect()
#3 /vagrant/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php(1017): Doctrine\DBAL\Connection->executeUpdate('CREATE DATABASE...')
#4 /vagrant/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php(430): Doctrine\DBAL\Schema\AbstractSchemaManager->_execSql('CREATE DATABASE...')
#5 /vagrant/vendor/doctrine/doctrine-bundle/Command/CreateDatabaseDoctrineCommand.php(89): Doctrine\DBAL\Schema\AbstractSchemaManager->createDatabase('`symfony`')
#6 /vagrant/vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php(259): Doctrine\Bundle\DoctrineBundle\Command\CreateDatabaseDoctrineCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#7 /vagrant/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php(886): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#8 /vagrant/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php(195): Symfony\Component\Console\Application->doRunCommand(Object(Doctrine\Bundle\DoctrineBundle\Command\CreateDatabaseDoctrineCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#9 /vagrant/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php(96): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#10 /vagrant/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php(126): Symfony\Bundle\FrameworkBundle\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#11 /vagrant/bin/symfony(20): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput))
#12 {main}

[kingcrunch]

OK, easier than expected. The dbname was still within url (as path). As long as the path was set was still used for the temporary connection. The entire url-parameter isn't required anymore at that point, because all relevant information was already extracted from the url before, so we can just drop it (and path, just to be sure).

[kingcrunch]

ping @guilhermeblanco (because you are the last one who merged a PR 😉 )

Can you have a look?

[kimhemsoe]

Tests ?

[kingcrunch]

@kimhemsoe Hi

The issue the PR tries to solve doesn't appear with SQLite, thus I cannot test it with the current setup. When you try to connect to a SQLite-Database, that doesn't exists, pdo_sqlite creates a new one in every case. There is no error, which this PR wants to prevent.

The solution would be to extend the tests to use a database server (MySQL, Postgresql, ..) instead of SQLite...

[j0k3r]

👎 For this without proper test.

I got an error when using SQLite.
Mostly because few lines below the updated unset(), there is a test on if (!isset($params['path'])) { which is now ALWAYS true.

// Need to get rid of _every_ occurrence of dbname from connection configuration and we have already extracted all relevant info from url
unset($params['dbname'], $params['path'], $params['url']);

$tmpConnection = DriverManager::getConnection($params);
$shouldNotCreateDatabase = $ifNotExists && in_array($name, $tmpConnection->getSchemaManager()->listDatabases());

// Only quote if we don't have a path
if (!isset($params['path'])) {
    $name = $tmpConnection->getDatabasePlatform()->quoteSingleIdentifier($name);
}

So the database name will ALWAYS be quoted and the generated name is wrong. Without your change, I got the database name of SQLite (which is a path):

string(58) "/Users/j0k/Sites/github/wallabag/app/cache/test/default.db"

With your change:

string(60) ""/Users/j0k/Sites/github/wallabag/app/cache/test/default.db""

Which is obviously wrong. So I can't create a database anymore. Doctrine keeps telling me:

Could not create database "/Users/j0k/Sites/github/wallabag/app/cache/test/default.db" for connection named default
An exception occured in driver: SQLSTATE[HY000] [14] unable to open database file

Edit:
I just saw there is a PR to fix that: #483

[kingcrunch]

Hi @j0k3r
Of course you are right, that the missing tests is a flaw, but to be fair I've pointed out, why I skipped the tests (insufficient infrastructure and therefore no time) and the test case wouldn't cover SQLite anyway, because the issue this PR tries to solve hadn't affected SQLite. Actually I wouldn't write the fix differently, because the intention is to get rid of all occurences of the database name, which means, I really don't want the path remaining anywhere.

However:
I had a short look at your issue and first I wondered, why this is an issue at all, because path is set for all (?) drivers and not just for the SQLite-driver, because path is the path from the URL, which is the database name for all other drivers. For the SQLite-driver it is a real path on the file system, but for all others it is an identifier. The longer I look at the three lines in question I wonder, what should it actually do... I wonder, because it does this:

• If path is not set, it will quote the database name. This is only the case, when the database name comes from the concrete parameters and is not overwritten by the url path. Here everything is OK so far.
• If path is set, it will not quote the database name. This is (of course) only the case, when the database name comes from the URL.

So why is it not needed to quote the databasoe name from the URL anymore?! Maybe somebody can explain me. My very very cautious gues is, that this was a fix for a different issue, that only accidentally solved the path issue for SQLite. Remember: Now database names for other drivers aren't quoted too, but this is rarely a problem, so probably nobody noticed.

Regarding a solution:
To me it seems, that looking at the path to differentiate between SQLite and all other drivers is the wrong approach (see above: Usually all other drivers have a path too). SQLite is a different kind of special case, like you can see in the DriverManager

       if (isset($url['path'])) {
            if (!isset($url['scheme']) || (strpos($url['scheme'], 'sqlite') !== false && $url['path'] == ':memory:')) {
                $params['dbname'] = $url['path']; // if the URL was just "sqlite::memory:", which parses to scheme and path only
            } else {
                $params['dbname'] = substr($url['path'], 1); // strip the leading slash from the URL
            }
        }

So either one can test, wether or not $params['dbname'][0] === '/' is true, or whether or not sqlite is used ($params['driver']). Later one seems to be cleaner imo ;)

[j0k3r]

I agree about every thing @kingcrunch :)

And the commit (20a3598) that add this check doesn't really describe what it should fix and why :

Don't put quotes around $name if it's a path

So we don't really know if this commit try to fix something related to SQLite or any other platform. This mean, we can't really rely on verifying what driver is used to quote database name. And since there is no test at all on that command, it's hard to rewrite the condition without side effects.

And this PR affect SQLite because I guess Doctrine is trying to create a file with this name "/Users/j0k/Sites/github/wallabag/app/cache/test/default.db" which of course cannot be created because of the quotes.

Instead of trying to rethink the way we should perform this verification, I think the solution in ##483 might be a good solution for you and me

[kingcrunch]

Hi @MarcelHernandez

Please read the comments (and see #483 )