Fix pg boolean conversion #625

Merged
merged 42 commits into from Jun 30, 2014

Projects

None yet

10 participants

Contributor

Merge of complementary PR's #527 and #564 + some fixes. Contributing it as an result of the (extended) phpconference.nl 2014 hackathon.

@lucasvanlierop lucasvanlierop changed the title from Fix pg boolean conversion to [WIP] Fix pg boolean conversion Jun 26, 2014
Member
stof commented Jun 26, 2014

This branch needs to be rebased as it conflicts with master (and changes shpoould probably be squashed a bit)

Owner

@lucasvanlierop thanks for taking care of this. Maybe I can catch you during the conference and we can discuss the PR together :-)

Contributor

@stof, PR is now up to date with master, I will squash the whole thing when it's finished.

@Ocramius, yes good idea, currently I'm at the point where I think boolean conversion deserves it's own class instead of having lots of extra methods in de platform class. But I don't know if you like that idea? Maybe we can use part of the lunch break to discuss it?

@Ocramius Ocramius and 1 other commented on an outdated diff Jun 27, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -2580,6 +2582,32 @@ public function convertBooleans($item)
return $item;
}
+
+ /**
+ * Some platforms have boolean literals that needs to be correctly converted
+ *
+ * The default conversion tries to convert value into bool "(bool)$item"
+ *
+ * @param mixed $item
+ *
+ * @return bool
@Ocramius Ocramius and 1 other commented on an outdated diff Jun 27, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
+ * The default conversion tries to convert value into bool "(bool)$item"
+ *
+ * @param mixed $item
+ *
+ * @return bool
+ */
+ public function convertFromBoolean($item)
+ {
+ return null === $item ? null: (bool) $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
Ocramius
Ocramius Jun 27, 2014 Owner

bool|bool[]? What are the actual supported values here?

lucasvanlierop
lucasvanlierop Jun 28, 2014 Contributor

Updating the actual param and return type is not possible, since all types of variables can be entered and the output can both be an integer (or a string in some inherited classes) OR the original input value if this was not a boolean or an array of it. So I kept the types but added some extra comments about this.

@Ocramius Ocramius commented on an outdated diff Jun 27, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
+ *
+ * @param mixed $item
+ *
+ * @return bool
+ */
+ public function convertFromBoolean($item)
+ {
+ return null === $item ? null: (bool) $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
Ocramius
Ocramius Jun 27, 2014 Owner

Same here, can you clarify the return type? mixed is kinda like a limbo

@Ocramius Ocramius commented on the diff Jun 27, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
+ 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)) {
+ return $callback(true);
+ }
+
+ /**
+ * Better safe than sorry: http://php.net/in_array#106319
Ocramius
Ocramius Jun 27, 2014 Owner

Missing joke

lucasvanlierop
lucasvanlierop Jun 29, 2014 Contributor

Haha, Ok I just kept most of the logic was it was.

@Ocramius Ocramius and 1 other commented on an outdated diff Jun 27, 2014
lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
*
+ * @param mixed $item
Ocramius
Ocramius Jun 27, 2014 Owner

as discussed, more precise types here

lucasvanlierop
lucasvanlierop Jun 29, 2014 Contributor

Same here, I would like to do it but not possible.

@Ocramius Ocramius and 1 other commented on an outdated diff Jun 27, 2014
...Doctrine/Tests/DBAL/Functional/Ticket/DBAL630Test.php
@@ -33,6 +33,12 @@ protected function tearDown()
{
if ($this->running) {
$this->_conn->getWrappedConnection()->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
+
+ // PDO::PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEMENT is deprecated in php 5.6. PDO::ATTR_EMULATE_PREPARES should
+ // be used instead. so should only it be set when it is supported.
+ if (version_compare('5.6', phpversion()) === 1) {
Ocramius
Ocramius Jun 27, 2014 Owner

if (PHP_VERSION_ID <= 50600) {

@Ocramius Ocramius and 1 other commented on an outdated diff Jun 27, 2014
...Doctrine/Tests/DBAL/Functional/Ticket/DBAL630Test.php
$this->_conn->getWrappedConnection()->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
+ // PDO::PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEMENT is deprecated in php 5.6. PDO::ATTR_EMULATE_PREPARES should
+ // be used instead. so should only it be set when it is supported.
+ if (version_compare('5.6', phpversion()) === 1) {
Ocramius
Ocramius Jun 27, 2014 Owner

Same as above

Contributor

@davividal and @birko I've merged both your PR's (#564 and #527) into one new PR handling both the conversion from Postgresql database boolean values to php and vice versa. Please check if it still is what you meant it to be. I'm just here to help out with longstanding open PR's.

@deeky666, Would you review this as you commented on the original PR's and also suggested part code @davividal has contributed.

@lucasvanlierop lucasvanlierop changed the title from [WIP] Fix pg boolean conversion to Fix pg boolean conversion Jun 29, 2014
Member

At the first glance this PR looks good so far. Something that we didn't talk about here is possible performance implications. I talked about this issue with @beberlei some time ago and he wasn't happy with all those callbacks. But I don't know really know if that has any impact on performance.
Otherwise I would like to hear from the originals authors again if they are fine with it and afterwards merge?

Contributor

@deeky666, thanks for taking a look. Well to be honest the conversion feels a bit over engineered to me but I tried to keep the focus on merging complementary solutions (and some cleaning up) and so I kept the logic more or less like it was.

Member

The original implementation had a lot of code duplication and was rather hard to read and understand. Thus I proposed a more "simplified" solution with smaller methods removing the code duplication but I see and understand that those callbacks are rather heavy here. TBH I don't mind which way to go :)
Thanks for putting both PRs together btw.

Contributor

Looks fine for me.

I don't know how to test performance of the callback implementation, sorry.

Contributor
birko commented Jun 30, 2014

Hmm for me probably ok. If I am looking correctly

Owner

@lucasvanlierop can you please remove lucasvanlierop/dbal@15d876f ? Please rebase instead.

birko and others added some commits Feb 12, 2014
@birko @lucasvanlierop birko Update AbstractPlatform.php
Default Boolean conversion behaviour
1c562fc
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php
added postgresql literals to Boolean conversion
47e0370
@birko @lucasvanlierop birko Update BooleanType.php
Changed to call platform specific Boolean conversion
1ae0bd8
@birko @lucasvanlierop birko Update AbstractPostgreSqlPlatformTestCase.php
added postgresql platform conversion test
d4becf6
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php
fixed bug when $item was set as bool true value
added 'off' literal
46241ad
@birko @lucasvanlierop birko Update AbstractPostgreSqlPlatformTestCase.php
added literals off and on to the testcase
4dfc8f1
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php
Changes after Ocramius comments
402c7c8
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php
fixed parameter order in conditions
ee87790
@birko @lucasvanlierop birko Update AbstractPlatform.php
Description fix
d901ba2
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php 7b161fa
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php c91e248
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php
updated convert function  to include uppercased values
c8b2bda
@birko @lucasvanlierop birko Update AbstractPostgreSqlPlatformTestCase.php
Updated test for uppercased values
efa4b3c
@birko @lucasvanlierop birko Update PostgreSqlPlatform.php fbba6d1
@birko @lucasvanlierop birko Update AbstractPlatform.php dbb0774
@birko @lucasvanlierop birko Update AbstractPlatform.php
reverted @deeky666 suggestion. Since it failed on testBooleanNullConvertsToPHPValue test. 
with message Failed asserting that true is null.
Since I moved this code from BoolenType::convertToPHPValue in the first place, I presume the code there correct.
86e5879
@davividal @lucasvanlierop davividal Working on PostgreSQL incorrect boolean handling when emulating prepa…
…red statements
b35b682
@davividal @lucasvanlierop davividal Complying with Doctrine PSR exceptions 2e98978
@davividal @lucasvanlierop davividal 'Unbreaking' public API 3b08681
@davividal @lucasvanlierop davividal Applying requested fixes 702967b
@davividal @lucasvanlierop davividal Forgot to update references ac28bbc
@davividal @lucasvanlierop davividal Renaming DbValue to DatabaseValue 37772e1
@davividal @lucasvanlierop davividal Checking for strings when converting to booleans 449055a
@davividal @lucasvanlierop davividal Handling all Postgres boolean literals 4090136
@davividal @lucasvanlierop davividal DRYing up tests and abstraction layer 08b8308
@davividal @lucasvanlierop davividal Improving conversion using @deeky666 idea d299565
@davividal @lucasvanlierop davividal Checking if the boolean string is valid 3de109f
@davividal @lucasvanlierop davividal Fixing indentation 59a5f5c
@lucasvanlierop lucasvanlierop Reduced nested complexity edda2d4
@lucasvanlierop lucasvanlierop Removed checks that are already covered by parent method 4e7e2df
@lucasvanlierop lucasvanlierop Used dataprovider for testConvertFromBoolean 03d1507
@lucasvanlierop lucasvanlierop Grouped all representations of boolean values into one dataprovider 9d73f30
@lucasvanlierop lucasvanlierop Used boolean literals definition 5edd7ea
@lucasvanlierop lucasvanlierop PG specific attribute PDO::PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEME…
…NT is now only set when it is still supported
ceb6643
@lucasvanlierop lucasvanlierop Use assertSame rather than assertEquals to prevent false positives du…
…e to type juggling.
240b64d
@lucasvanlierop lucasvanlierop only set deprecated attribute in older php versions so it does not br…
…eak the test in php 5.6(+)
0458eaf
@lucasvanlierop lucasvanlierop Fixed incorrectly spelled constant 0753bac
@lucasvanlierop lucasvanlierop Do not use version compare anymore 7298404
@lucasvanlierop lucasvanlierop Corrected return value in docblock d10eb56
@lucasvanlierop lucasvanlierop Reverted indentation change 8c636d4
@lucasvanlierop lucasvanlierop Added some extra param/return value information d98ac8c
@lucasvanlierop lucasvanlierop Removed duplicate method 871089c
Contributor

@Ocramius: removed the merge commit, waiting for travis.

Owner

I'll merge right after that, thanks :-)

@Ocramius Ocramius self-assigned this Jun 30, 2014
Contributor

Meh... somehow one of the test crashed (not failed) https://travis-ci.org/doctrine/dbal/jobs/28779614

Member

@lucasvanlierop restarted the job.

@Ocramius Ocramius merged commit 5eb36c7 into doctrine:master Jun 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@Ocramius Ocramius referenced this pull request Jun 30, 2014
Closed

Boolean conversion #527

Owner

Merged, solved, thanks!

Contributor

Cool, hope it will work out well :-)

Owner

@lucasvanlierop I'm still in AMS and I have a pitchfork if it doesn't :P j/k

@sergeyz sergeyz added a commit to sergeyz/dbal that referenced this pull request Jul 22, 2014
@sergeyz sergeyz Backport #625 1de27dd
@sergeyz sergeyz added a commit to sergeyz/dbal that referenced this pull request Jul 23, 2014
@sergeyz sergeyz Backport #625 f3c9439
edigu commented Oct 27, 2014

This problem still continues in PHP 5.6. I think it needs more specific test. See #632

Owner
Ocramius commented Nov 5, 2014

Probably related to #714

Contributor
mbeccati commented Nov 6, 2014

Possibly caused by https://bugs.php.net/bug.php?id=68351 - which I've just fixed (sorry I haven't read the full thread here ;) )

Member
deeky666 commented Nov 7, 2014

@mbeccati this reads promising. Does it also apply to \PDOStatement::bindValue()? Could you maybe add a test for null values? Because this is what's going wrong here. Inserting a null value into a nullable boolean column with emulated prepares enabled and using \PDOStatement::bindValue(1, null, \PDO::PARAM_BOOL) result in PHP converting null to true in PHP 5.6 while it was converted to false in PHP < 5.6.

Contributor
mbeccati commented Nov 7, 2014

@deeky666 yes, that's precisely what the fix is for. The bug existed in previous versions as well, but it was probably less likely to trigger it.

Member
deeky666 commented Nov 7, 2014

@mbeccati awesome! Thanks a lot for taking care of that issue. It has been causing us bad headaches for some time now...

@stof stof commented on the diff Nov 13, 2014
lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
@@ -686,6 +708,71 @@ 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)
+ {
+ if (null === $value) {
+ return $callback(false);
stof
stof Nov 13, 2014 Member

you should use call_user_func to support all callables on PHP 5.3

stof
stof Nov 13, 2014 Member

hmm, given it is a private method, it may not be needed though

Ocramius
Ocramius Nov 13, 2014 Owner

Maybe type-hint Closure?

@Ocramius Ocramius referenced this pull request in wmde/WikibaseQueryEngine Nov 27, 2014
Merged

Fix BooleanHandler for PostgreSQL #140

edigu commented Dec 22, 2014

@mbeccati The PDO::PARAM_BOOL and ATTR_EMULATE_PREPARES misbehaving bug is fixed in 5.6.4 and 5.5.20. See: https://bugs.php.net/bug.php?id=68351

Owner

@edigu awesome! :-)

@deeky666 do you think we should make DBAL conflict with >=5.6.0,<=5.6.3?

Contributor

@Ocramius @deeky666 For reference, the PHP bug I fixed was there since https://bugs.php.net/bug.php?id=62593 was fixed - i.e. 5.4.9+ / 5.3.19+. I'm not even sure what was the behaviour before that.

Member
stof commented Dec 26, 2014

@Ocramius I don't think we should:

  • we don't exclude all PHP versions affected by a bug, otherwise people will just be unable to install the package in many cases. Doing it for a single bug would be inconsistent
  • it only affects the PostgreSQL driver, so why forbidding to use 5.6.0 for people using MySQL ?
Owner

why forbidding to use 5.6.0 for people using MySQL ?

Because of unforseen consequences there ("wtf" moments for pgsql users, mysql is fine). Anyway, users are still free to upgrade, and I agree with you on the users that are not affected by the bug.

I guess the only required change is a travis upgrade of PHP 5.6 (once available)

Member

We could just document this incompatibility somewhere. Otherwise the only solution I see is to make PDO_PGSQL not use emulated prepares for those versions to stay compatible. But that would also require adjusting the PDO implementation DBAL wise....

Contributor

Just to confirm that upgrading PHP from 5.6.3 to 5.6.4 indeed solved that bug for me without any other changes.

Also I agree with @deeky666 that it should be documented somewhere as known bugs.

Member

I'd say let's wait for travis to go for 5.6.4, see the build result and then add a section in the documentation. Not going too complicated here, we cannot deal with everything....

Contributor

Hello,
Since DBAL v2.4.3 we can't handle null value on boolean field : false is returned instead of null.
Is this normal ?

edigu commented Feb 27, 2015

@Vincent-Simonin what is your php version?

Contributor

@edigu v5.5.22

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