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

Added parameter default_dbname to pdo_pgsql driver, used to override the default database #2284

Merged
merged 5 commits into from
Jan 15, 2016
Merged

Added parameter default_dbname to pdo_pgsql driver, used to override the default database #2284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ceadc0
Added parameter "default_dbname" to pdo_pgsql driver which can be use…
kimhemsoe Jan 8, 2016
c31de12
Added whitespace
kimhemsoe Jan 8, 2016
d7bbd30
Always connect postgres schema if dbname or default_dbname is not pro…
kimhemsoe Jan 8, 2016
f471a0f
Added tests for all variants
kimhemsoe Jan 11, 2016
253b602
Fixed indenting after rebase
kimhemsoe Jan 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/en/reference/configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,8 @@ pdo\_pgsql
- ``dbname`` (string): Name of the database/schema to connect to.
- ``charset`` (string): The charset used when connecting to the
database.
- ``default_dbname`` (string): Override the default database (postgres)
to connect to.
- ``sslmode`` (string): Determines whether or with what priority
a SSL TCP/IP connection will be negotiated with the server.
See the list of available modes:
Expand Down
4 changes: 3 additions & 1 deletion lib/Doctrine/DBAL/Driver/PDOPgSql/Driver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,12 @@ private function _constructPdoDsn(array $params)

if (isset($params['dbname'])) {
$dsn .= 'dbname=' . $params['dbname'] . ' ';
} elseif (isset($params['default_dbname'])) {
$dsn .= 'dbname=' . $params['default_dbname'] . ' ';
Copy link
Member

Choose a reason for hiding this comment

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

We should still connect to postgres if the parameter is not set as we will have errors again otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, postgres is the default if dont provide one. Ive tested it.

Copy link
Member

Choose a reason for hiding this comment

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

@kimhemsoe that is not true. It defaults to a database that is named equally to the connection user. This is what originally caused trouble. See PHP issue and doctrine/DoctrineBundle#402

Copy link
Member

Choose a reason for hiding this comment

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

@deeky666 @kimhemsoe Ah yes, I was wrong - you can verify this using:

PDO

php -r "$pdo = new PDO('pgsql:host=localhost port=5432', 'your_username', 'your_password');"

If no db matching your username exists, you get:

PDOException: SQLSTATE[08006] [7] FATAL:  database "your_username" does not exist in Command line code on line 1

Call Stack:
    0.0000     125096   1. {main}() Command line code:0
    0.0000     125520   2. PDO->__construct() Command line code:1

pgsql

php -r "$conn = pg_connect('host=localhost port=5432 user=your_username password=your_password');"

If no db matching your username exists, you get:

Warning: pg_connect(): Unable to connect to PostgreSQL server: FATAL:  database "your_username" does not exist in Command line code on line 1

Call Stack:
    0.0000     124792   1. {main}() Command line code:0
    0.0000     124928   2. pg_connect() Command line code:1

} else {
Copy link
Member

Choose a reason for hiding this comment

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

I'd say remove this else block entirely, as postgresql has a default behavior, which is to connect to the database matching the username.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rather want to remove the "else", that way we get the default behaviour (username) but people can still override it something like template1 if they want to live on the edge.

Copy link
Member

Choose a reason for hiding this comment

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

Agree - I meant remove L91-96.

Copy link
Member

Choose a reason for hiding this comment

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

Strongly disagree. As already said this is basically what it's all about. Defining default_dbname should be the last resort option in case the database server setup doesn't have the postgres database. When going back to the old behaviour, we move the action to be taken to the user which is not good IMO. DBAL should try everything possible first before the user is asked to save the world.

// Used for temporary connections to allow operations like dropping the database currently connected to.
// Connecting without an explicit database does not work, therefore "postgres" database is used
// as it is certainly present in every server setup.
// as it is mostly present in every server setup.
$dsn .= 'dbname=postgres' . ' ';
}

Expand Down
39 changes: 39 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Driver/PDOPgSql/DriverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

namespace Doctrine\Tests\DBAL\Functional\Driver\PDOPgSql;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\PDOPgSql\Driver;
use Doctrine\Tests\DBAL\Functional\Driver\AbstractDriverTest;
use Doctrine\Tests\TestUtil;

class DriverTest extends AbstractDriverTest
{
Expand All @@ -20,6 +22,43 @@ protected function setUp()
}
}

/**
* @dataProvider getDatabaseParameter
*/
public function testDatabaseParameters($databaseName, $defaultDatabaseName, $expectedDatabaseName)
{
$params = $this->_conn->getParams();
$params['dbname'] = $databaseName;
$params['default_dbname'] = $defaultDatabaseName;

$connection = new Connection(
$params,
$this->_conn->getDriver(),
$this->_conn->getConfiguration(),
$this->_conn->getEventManager()
);

$this->assertSame(
$expectedDatabaseName,
$this->driver->getDatabase($connection)
);
}

public function getDatabaseParameter()
{
$params = TestUtil::getConnection()->getParams();
$realDatabaseName = $params['dbname'];
$dummyDatabaseName = $realDatabaseName . 'a';

return array(
// dbname, default_dbname, expected
array($realDatabaseName, null, $realDatabaseName),
array($realDatabaseName, $dummyDatabaseName, $realDatabaseName),
array(null, $realDatabaseName, $realDatabaseName),
array(null, null, $this->getDatabaseNameForConnectionWithoutDatabaseNameParameter()),
);
}

/**
* @group DBAL-1146
*/
Expand Down