Skip to content

[DBAL-630] Incorrect PostgreSQL boolean handling #564

Closed
wants to merge 13 commits into from

6 participants

@davividal

Working on PostgreSQL incorrect boolean handling when emulating prepared statements

@doctrinebot

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-863

We use Jira to track the state of pull requests and the versions they got
included in.

@davividal

The only issue with this PR is that it breaks Doctrine2 ORM. I already have a patch for that too, but I'm not sure on how to proceed.

@deeky666 deeky666 commented on an outdated diff Apr 4, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
* @return mixed
*/
- public function convertBooleans($item)
+ public function convertBoolToSqlLiteral($item)
@deeky666
Doctrine member
deeky666 added a note Apr 4, 2014

Please don't change the public API here, this breaks BC and is unacceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Apr 4, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -2367,6 +2369,18 @@ public function convertBooleans($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 convertBoolToDbValue($item)
+ {
+ return self::convertBooleans($item);
@stof
Doctrine member
stof added a note Apr 4, 2014

please use $this->, not self:: as you are refering to an instance method, not to a static method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Apr 4, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -2367,6 +2369,18 @@ public function convertBooleans($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 convertBoolToDbValue($item)
@stof
Doctrine member
stof added a note Apr 4, 2014

please use booleans, not bool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Apr 4, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
+
+ /**
+ * {@inheritDoc}
+ */
+ public function convertBoolToDbValue($item)
+ {
+ if ( ! $this->useBooleanTrueFalseStrings) {
+ return parent::convertBoolToDbValue($item);
+ }
+
+ if (is_array($item)) {
+ foreach ($item as $key => $value) {
+ if (is_bool($value) || is_numeric($value)) {
+ $item[$key] = $value ? 1 : 0;
+ } elseif (is_string($value)) {
+ $item[$key] = (trim(strtolower($value)) === 'false');
@stof
Doctrine member
stof added a note Apr 4, 2014

This is weird. You have a real boolean here, while the code uses 1 and 0 just above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Apr 4, 2014
...Doctrine/Tests/DBAL/Functional/Ticket/DBAL630Test.php
@@ -60,12 +61,15 @@ public function testBooleanConversionBoolParamRealPrepares()
public function testBooleanConversionBoolParamEmulatedPrepares()
{
- $this->markTestIncomplete('There is something missing here, on some machines it fails on some it passes.');
+ // $this->markTestIncomplete('There is something missing here, on some machines it fails on some it passes.');
@stof
Doctrine member
stof added a note Apr 4, 2014

if the test is not incomplete anymore, this should be removed, not commented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 4, 2014
...DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php
{
$platform = $this->createPlatform();
$platform->setUseBooleanTrueFalseStrings(false);
- $this->assertEquals('1', $platform->convertBooleans(true));
- $this->assertEquals('0', $platform->convertBooleans(false));
+ $this->assertEquals('1', $platform->convertBoolToSqlLiteral(true));
@stof
Doctrine member
stof added a note Apr 4, 2014

this looks wrong to me

@davividal
davividal added a note Apr 4, 2014

It is exactly like it has ever been.

@stof
Doctrine member
stof added a note Apr 4, 2014

note at the time I commented on it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davividal

Any more thoughts on this?

@deeky666 deeky666 and 1 other commented on an outdated diff Apr 9, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
}
}
} else {
- if (is_bool($item) || is_numeric($item)) {
- $item = ($item) ? 'true' : 'false';
- }
+ if (is_bool($item) || is_numeric($item)) {
+ $item = ($item) ? 'true' : 'false';
+ } elseif (is_string($item)) {
+ $item = trim(strtolower($item));
+ if ('false' !== $item && 'f' !== $item) {
@deeky666
Doctrine member
deeky666 added a note Apr 9, 2014

Is it maybe necessary to take the other valid boolean literals like n, no, off etc (and opposites) into account which are mentioned in the documentation? I'm not sure about this...

@davividal
davividal added a note Apr 9, 2014

I haven't thought about it.

What do you think about a private attribute called $booleansLiterals?

$booleansLiterals = ['true' => ['t', 'true', ...], 'false' => ['f', 'false', ...]];
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deeky666
Doctrine member
deeky666 commented Apr 9, 2014

Btw, is this PR complementary to #527 ?

@davividal

@deeky666 it looks like this PR complements #527 . How should I proceed?

@davividal davividal commented on the diff Apr 9, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
@@ -716,14 +738,62 @@ public function convertBooleans($item)
@davividal
davividal added a note Apr 9, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davividal

@deeky666 @stof any other thoughts on this?

@deeky666 deeky666 commented on an outdated diff Apr 23, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
+ }
+ }
+
+ return $item;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function convertBooleansToDatabaseValue($item)
+ {
+ if ( ! $this->useBooleanTrueFalseStrings) {
+ return parent::convertBooleansToDatabaseValue($item);
+ }
+
+ if (is_array($item)) {
@deeky666
Doctrine member
deeky666 added a note Apr 23, 2014

This whole if block is exactly the same as in convertBooleans() except for the values of true and false. Would be nice to avoid code duplication through another private method here IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deeky666 deeky666 commented on an outdated diff Apr 23, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
}
}
} else {
- if (is_bool($item) || is_numeric($item)) {
- $item = ($item) ? 'true' : 'false';
- }
+ if (is_bool($item) || is_numeric($item)) {
@deeky666
Doctrine member
deeky666 added a note Apr 23, 2014

Also this if block is the same as the one from the previous block. Could be extracted, too IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deeky666 deeky666 commented on an outdated diff Apr 23, 2014
...DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php
$this->assertEquals('false', $platform->convertBooleans(false));
+ $this->assertEquals('false', $platform->convertBooleans('f'));
+ $this->assertEquals('false', $platform->convertBooleans('false'));
+ $this->assertEquals('false', $platform->convertBooleans('n'));
+ $this->assertEquals('false', $platform->convertBooleans('no'));
+ $this->assertEquals('false', $platform->convertBooleans('off'));
+ $this->assertEquals('false', $platform->convertBooleans('0'));
@deeky666
Doctrine member
deeky666 added a note Apr 23, 2014

Could you please use a data provider here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deeky666
Doctrine member

Despite my suggested improvements I am fine with this PR but TBH I don't have a clue about emulated prepared statements in PostgreSQL and I would like to hear someone else's meaning about the validity of this fix. Otherwise it looks reasonable to me.

@deeky666
Doctrine member

@beberlei you have made the first step on this issue. Thoughts?

@davividal

@deeky666 I did a refactoring, what do you think?

@deeky666
Doctrine member

not quite what I thought. Here is my suggestion (not tested):

/**
 * 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)
{
    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}
 */
public function convertBooleans($item)
{
    if ( ! $this->useBooleanTrueFalseStrings) {
        return parent::convertBooleans($item);
    }

    return $this->doConvertBooleans(
        $item,
        function ($boolean) {
            return true === $boolean ? 'true' : 'false';
        }
    );
}

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

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

@deeky666 it works. Your solution is beautiful!

@deeky666 deeky666 and 1 other commented on an outdated diff Apr 23, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
+ 't',
+ 'true',
+ 'y',
+ 'yes',
+ 'on',
+ '1'
+ ),
+ 'false' => array(
+ 'f',
+ 'false',
+ 'n',
+ 'no',
+ 'off',
+ '0'
+ )
+ );
@deeky666
Doctrine member
deeky666 added a note Apr 23, 2014

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?

@deeky666
Doctrine member
deeky666 added a note Apr 23, 2014

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.

@davividal
davividal added a note Apr 23, 2014

"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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@deeky666
Doctrine member

@Ocramius can you please have a look at this implementation. I forgot to keep in mind that those methods could be performance sensitive due being used in DBAL type conversion. Do you think we have to undDRY this thing again?

@davividal

@deeky666 @Ocramius @stof can I help with anything to get this merged?

@deeky666
Doctrine member
deeky666 commented May 5, 2014

Still waiting for feedback on possible performance implications.
@Ocramius ping

@davividal

Please let me know if I can help with some benchmarking.

@deeky666
Doctrine member
deeky666 commented May 5, 2014

@davividal Sure go ahead and do so. Then we don't have to speculate about that :) Basically the conversion via BooleanType is important to know about.

@davividal

Sorry, I wasn't able to do any progress on this.

If anyone could help, I don't know when I will be able to (re-)take this.

@lucasvanlierop lucasvanlierop commented on the diff Jun 26, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
@@ -704,6 +726,69 @@ protected function _getCreateTableSQL($tableName, array $columns, array $options
}
/**
+ * 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)
+ {

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}'");
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lucasvanlierop

I've been comparing this PR with the #527 which seems almost finished. And I feel it would be best if that PR is finished and merged soon and the result is merged into this one. It does not do exactly the same but there is overlap and I think it's best if conversion in both directions both work more or less the same way.

@davividal davividal Simplifying code
Kudos to Lucas van Lierop for the refactoring
956d5a7
@Ocramius
Doctrine member

Closing - continuing in #625

@Ocramius Ocramius closed this Jun 27, 2014
@Ocramius Ocramius self-assigned this Jul 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.