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

[DBAL-630] Incorrect PostgreSQL boolean handling #564

Closed
wants to merge 13 commits into from
Closed

[DBAL-630] Incorrect PostgreSQL boolean handling #564

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4736075
Working on PostgreSQL incorrect boolean handling when emulating prepa…
davividal Apr 4, 2014
0b4a087
Complying with Doctrine PSR exceptions
davividal Apr 4, 2014
63d3e88
'Unbreaking' public API
davividal Apr 4, 2014
a73268d
Applying requested fixes
davividal Apr 4, 2014
17e9deb
Forgot to update references
davividal Apr 4, 2014
c3df91d
Renaming DbValue to DatabaseValue
davividal Apr 4, 2014
cf44e4b
Checking for strings when converting to booleans
davividal Apr 4, 2014
038c524
Handling all Postgres boolean literals
davividal Apr 9, 2014
c8ffb84
DRYing up tests and abstraction layer
davividal Apr 23, 2014
a328758
Improving conversion using @deeky666 idea
davividal Apr 23, 2014
f68c998
Checking if the boolean string is valid
davividal Apr 23, 2014
30589a5
Fixing indentation
davividal Apr 23, 2014
956d5a7
Simplifying code
davividal Jun 26, 2014
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
16 changes: 15 additions & 1 deletion lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
View file
Open in desktop
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
106 changes: 96 additions & 10 deletions lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
View file
Open in desktop
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'
)
);
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation. You have 4 spaces to much here from below the array opener. Also I wonder if we even need this map. If you look at your current implementation, we basically only DO and NEED TO check for values evaluating to false, no? Everything else being converted is true anyways. Like if I pass an object or a resource or anything else (stupid but possible) that is not null, false, 0, 'f', 'false', 'n', 'no', 'off' or '0'. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise if we would strictly stick tho this map we would have to throw exceptions for unexpected values which we cannot do IMO because that might break BC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Smart" indent, sorry.

I'm not sure. Initially, I was checking only for false values and converting everything else to true. But if I'm using Postgres boolean literals for true, I will use one of these six. If I'm inputing a literal for true different than these, I would expect an error.

This is Postgres behavior btw:

ERROR:  invalid input syntax for type boolean: "my-awesome-true"
LINE 1: SELECT 'my-awesome-true'::BOOLEAN

As for what do I think: I really think that false should be equal to 0, false or null, true otherwise, like C. Diverging from that you get Ruby awesome 0 equals to true. :) [/troll]


/**
* PostgreSQL has different behavior with some drivers
* with regard to how booleans have to be handled.
Expand Down Expand Up @@ -703,6 +725,60 @@ 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) && in_array(trim(strtolower($value)), $this->booleanLiterals['false'])) {
return $callback(false);
}

return $callback(true);
}

/**
* 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 +790,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
View file
Open in desktop
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
View file
Open in desktop
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']);
}
}
88 changes: 82 additions & 6 deletions tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php
View file
Open in desktop
Expand Up @@ -291,25 +291,51 @@ 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));
}

public function testGetCreateSchemaSQL()
Expand Down Expand Up @@ -521,4 +547,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'),
);
}
}