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.

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

[DBAL-630] Incorrect PostgreSQL boolean handling #564

Closed
wants to merge 13 commits into from
16 changes: 15 additions & 1 deletion lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
Expand Up @@ -2347,8 +2347,10 @@ public function prefersIdentityColumns()
*
* The default conversion in this implementation converts to integers (false => 0, true => 1).
*
* @param mixed $item
* There are two contexts when converting booleans: Literals and Prepared Statements.
* This method should handle the literal case
*
* @param mixed $item
* @return mixed
*/
public function convertBooleans($item)
Expand All @@ -2366,6 +2368,18 @@ public function convertBooleans($item)
return $item;
}

/**
* This method should handle the prepared statements case. When there is no
* distinction, it's OK to use the same method.
*
* @param mixed $item
* @return mixed
*/
public function convertBooleansToDatabaseValue($item)
{
return $this->convertBooleans($item);
}

/**
* Returns the SQL specific for the platform to get the current date.
*
Expand Down
115 changes: 105 additions & 10 deletions lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
Expand Up @@ -42,6 +42,28 @@ class PostgreSqlPlatform extends AbstractPlatform
*/
private $useBooleanTrueFalseStrings = true;

/**
* @var array PostgreSQL booleans literals
*/
private $booleanLiterals = array(
'true' => array(
't',
'true',
'y',
'yes',
'on',
'1'
),
'false' => array(
'f',
'false',
'n',
'no',
'off',
'0'
)
);

/**
* PostgreSQL has different behavior with some drivers
* with regard to how booleans have to be handled.
Expand Down Expand Up @@ -703,6 +725,69 @@ protected function _getCreateTableSQL($tableName, array $columns, array $options
return $sql;
}

/**
* Converts a single boolean value.
*
* First converts the value to its native PHP boolean type
* and passes it to the given callback function to be reconverted
* into any custom representation.
*
* @param mixed $value The value to convert.
* @param callable $callback The callback function to use for converting the real boolean value.
*
* @return mixed
*/
private function convertSingleBooleanValue($value, $callback)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

To reduce nested complexity this can be rewritten as:

if (null === $value) {
    return $callback(false);
}

if (is_bool($value) || is_numeric($value)) {
    return $callback($value ? true : false);
}

if (!is_string($value)) {
    return $callback(true);
}

/**
 * Better safe than sorry: http://php.net/in_array#106319
 */
if (in_array(trim(strtolower($value)), $this->booleanLiterals['false'], true)) {
    return $callback(false);
} 

if (in_array(trim(strtolower($value)), $this->booleanLiterals['true'], true)) {
    return $callback(true);
}

throw new \UnexpectedValueException("Unrecognized boolean literal '${value}'");

if (null === $value) {
return $callback(false);
}

if (is_bool($value) || is_numeric($value)) {
return $callback($value ? true : false);
}

if ( ! is_string($value)) {
return $callback(true);
}

/**
* Better safe than sorry: http://php.net/in_array#106319
*/
if (in_array(trim(strtolower($value)), $this->booleanLiterals['false'], true)) {
return $callback(false);
} elseif (in_array(trim(strtolower($value)), $this->booleanLiterals['true'], true)) {
return $callback(true);
} else {
throw new \UnexpectedValueException("Unrecognized boolean literal '${value}'");
}
}

/**
* Converts one or multiple boolean values.
*
* First converts the value(s) to their native PHP boolean type
* and passes them to the given callback function to be reconverted
* into any custom representation.
*
* @param $item The value(s) to convert.
* @param $callback The callback function to use for converting the real boolean value(s).
*
* @return mixed
*/
private function doConvertBooleans($item, $callback)
{
if (is_array($item)) {
foreach ($item as $key => $value) {
$item[$key] = $this->convertSingleBooleanValue($value, $callback);
}

return $item;
}

return $this->convertSingleBooleanValue($item, $callback);
}

/**
* {@inheritDoc}
*
Expand All @@ -714,19 +799,29 @@ public function convertBooleans($item)
return parent::convertBooleans($item);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're not using TrueFalseStrings, convertBooleans('t') won't convert it to neither 1 nor 0.
The same goes for almost every case described in PostgreSQL documentation.

if (is_array($item)) {
foreach ($item as $key => $value) {
if (is_bool($value) || is_numeric($item)) {
$item[$key] = ($value) ? 'true' : 'false';
}
return $this->doConvertBooleans(
$item,
function ($boolean) {
return true === $boolean ? 'true' : 'false';
}
} else {
if (is_bool($item) || is_numeric($item)) {
$item = ($item) ? 'true' : 'false';
}
);
}

/**
* {@inheritDoc}
*/
public function convertBooleansToDatabaseValue($item)
{
if ( ! $this->useBooleanTrueFalseStrings) {
return parent::convertBooleansToDatabaseValue($item);
}

return $item;
return $this->doConvertBooleans(
$item,
function ($boolean) {
return (int) $boolean;
}
);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Types/BooleanType.php
Expand Up @@ -41,7 +41,7 @@ public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $pla
*/
public function convertToDatabaseValue($value, AbstractPlatform $platform)
{
return $platform->convertBooleans($value);
return $platform->convertBooleansToDatabaseValue($value);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions tests/Doctrine/Tests/DBAL/Functional/Ticket/DBAL630Test.php
Expand Up @@ -33,6 +33,7 @@ protected function tearDown()
{
if ($this->running) {
$this->_conn->getWrappedConnection()->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$this->_conn->getWrappedConnection()->setAttribute(PDO::PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEMENT, false);
}
}

Expand Down Expand Up @@ -60,12 +61,13 @@ public function testBooleanConversionBoolParamRealPrepares()

public function testBooleanConversionBoolParamEmulatedPrepares()
{
$this->markTestIncomplete('There is something missing here, on some machines it fails on some it passes.');

$this->_conn->getWrappedConnection()->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
$this->_conn->getWrappedConnection()->setAttribute(PDO::PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEMENT, true);

$platform = $this->_conn->getDatabasePlatform();

$stmt = $this->_conn->prepare('INSERT INTO dbal630 (bool_col) VALUES(?)');
$stmt->bindValue(1, 'false', PDO::PARAM_BOOL);
$stmt->bindValue(1, $platform->convertBooleansToDatabaseValue('false'), PDO::PARAM_BOOL);
$stmt->execute();

$id = $this->_conn->lastInsertId('dbal630_id_seq');
Expand All @@ -74,6 +76,6 @@ public function testBooleanConversionBoolParamEmulatedPrepares()

$row = $this->_conn->fetchAssoc('SELECT bool_col FROM dbal630 WHERE id = ?', array($id));

$this->assertTrue($row['bool_col']);
$this->assertFalse($row['bool_col']);
}
}
Expand Up @@ -291,25 +291,60 @@ protected function getQuotedColumnInForeignKeySQL()

/**
* @group DBAL-457
* @dataProvider pgStringBooleanProvider
*/
public function testConvertBooleanAsStrings()
public function testConvertBooleanAsLiteralStrings($expected, $input)
{
$platform = $this->createPlatform();

$this->assertEquals('true', $platform->convertBooleans(true));
$this->assertEquals('false', $platform->convertBooleans(false));
$this->assertEquals($expected, $platform->convertBooleans($input));
}

/**
* @group DBAL-457
*/
public function testConvertBooleanAsIntegers()
public function testConvertBooleanAsLiteralIntegers()
{
$platform = $this->createPlatform();
$platform->setUseBooleanTrueFalseStrings(false);

$this->assertEquals('1', $platform->convertBooleans(true));
$this->assertEquals('0', $platform->convertBooleans(false));
$this->assertEquals(1, $platform->convertBooleans(true));
$this->assertEquals(1, $platform->convertBooleans('1'));

$this->assertEquals(0, $platform->convertBooleans(false));
$this->assertEquals(0, $platform->convertBooleans('0'));
}

/**
* @group DBAL-630
* @dataProvider pgStringBooleanDatabaseValueProvider
*/
public function testConvertBooleanAsDatabaseValueStrings($expected, $input)
{
$platform = $this->createPlatform();

$this->assertEquals($expected, $platform->convertBooleansToDatabaseValue($input));
}

/**
* @group DBAL-630
*/
public function testConvertBooleanAsDatabaseValueIntegers()
{
$platform = $this->createPlatform();
$platform->setUseBooleanTrueFalseStrings(false);

$this->assertEquals(1, $platform->convertBooleansToDatabaseValue(true));
$this->assertEquals(0, $platform->convertBooleansToDatabaseValue(false));
}

/**
* @expectedException UnexpectedValueException
* @expectedExceptionMessage Unrecognized boolean literal 'my-bool'
*/
public function testThrowsExceptionWithInvalidBooleanLiteral()
{
$platform = $this->createPlatform()->convertBooleansToDatabaseValue("my-bool");
}

public function testGetCreateSchemaSQL()
Expand Down Expand Up @@ -521,4 +556,54 @@ protected function getQuotedAlterTableRenameIndexSQL()
'ALTER INDEX "foo" RENAME TO "bar"',
);
}

/**
* PostgreSQL boolean strings provider
* @return array
*/
public function pgStringBooleanProvider()
{
return array(
array('true', true),
array('true', 't'),
array('true', 'true'),
array('true', 'y'),
array('true', 'yes'),
array('true', 'on'),
array('true', '1'),

array('false', false),
array('false', 'f'),
array('false', 'false'),
array('false', 'n'),
array('false', 'no'),
array('false', 'off'),
array('false', '0'),
);
}

/**
* PostgreSQL boolean strings as database values provider
* @return array
*/
public function pgStringBooleanDatabaseValueProvider()
{
return array(
array(1, true),
array(1, 't'),
array(1, 'true'),
array(1, 'y'),
array(1, 'yes'),
array(1, 'on'),
array(1, '1'),

array(0, false),
array(0, 'f'),
array(0, 'false'),
array(0, 'n'),
array(0, 'no'),
array(0, 'off'),
array(0, '0'),
);
}
}