Boolean conversion #527

Closed
wants to merge 18 commits into
from

Conversation

Projects
None yet
6 participants
@birko
Contributor

birko commented Feb 12, 2014

Added platform specific conversion form database Boolean type to PHP bool type

birko added some commits Feb 12, 2014

Update AbstractPlatform.php
Default Boolean conversion behaviour
Update PostgreSqlPlatform.php
added postgresql literals to Boolean conversion
Update BooleanType.php
Changed to call platform specific Boolean conversion
Update AbstractPostgreSqlPlatformTestCase.php
added postgresql platform conversion test
@doctrinebot

This comment has been minimized.

Show comment Hide comment
@doctrinebot

doctrinebot Feb 12, 2014

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-811

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

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-811

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

birko added some commits Feb 12, 2014

Update PostgreSqlPlatform.php
fixed bug when $item was set as bool true value
added 'off' literal
Update AbstractPostgreSqlPlatformTestCase.php
added literals off and on to the testcase
+ */
+ public function convertFromBoolean($item)
+ {
+ if(in_array((string)$item, array('false', 'f', 'n', 'no', 'off')))

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

Can you fix the CS here? We follow PSR-2 with some minor additions

@Ocramius

Ocramius Feb 14, 2014

Member

Can you fix the CS here? We follow PSR-2 with some minor additions

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

You can avoid the call to in_array if the returned value is either null, true or false

@Ocramius

Ocramius Feb 14, 2014

Member

You can avoid the call to in_array if the returned value is either null, true or false

birko added some commits Feb 14, 2014

Update PostgreSqlPlatform.php
Changes after Ocramius comments
Update PostgreSqlPlatform.php
fixed parameter order in conditions
@birko

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 14, 2014

Contributor

@Ocramius hope I made the changes correctly in the way you wanted

Contributor

birko commented Feb 14, 2014

@Ocramius hope I made the changes correctly in the way you wanted

+ */
+ public function convertFromBoolean($item)
+ {
+ if((null !== $item) && (false !== $item) && (true !== $item) && in_array((string)$item, array('false', 'f', 'n', 'no', 'off')))

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

Break this line

@Ocramius

Ocramius Feb 14, 2014

Member

Break this line

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 14, 2014

Contributor

you mean the "if" statement?

@birko

birko Feb 14, 2014

Contributor

you mean the "if" statement?

+ public function convertFromBoolean($item)
+ {
+ if((null !== $item) && (false !== $item) && (true !== $item) && in_array((string)$item, array('false', 'f', 'n', 'no', 'off')))
+ {

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

The CS is still to be fixed

@Ocramius

Ocramius Feb 14, 2014

Member

The CS is still to be fixed

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 14, 2014

Contributor

What exactly needs to be fixed?

@birko

birko Feb 14, 2014

Contributor

What exactly needs to be fixed?

+ }
+ else
+ {
+ return parent::convertFromBoolean($item);

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

Do you need the parent impl?

@Ocramius

Ocramius Feb 14, 2014

Member

Do you need the parent impl?

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 14, 2014

Contributor

Yes. I wanted to to have the default behavior as fallback option

@birko

birko Feb 14, 2014

Contributor

Yes. I wanted to to have the default behavior as fallback option

@birko

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 14, 2014

Contributor

Ok now I don't know what more I could do for the CS.

Contributor

birko commented Feb 14, 2014

Ok now I don't know what more I could do for the CS.

+ in_array((string)$item, array('false', 'f', 'n', 'no', 'off'))
+ ) {
+ return false;
+ } else {

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

You can avoid the else clause completely, since you're returning early

@Ocramius

Ocramius Feb 14, 2014

Member

You can avoid the else clause completely, since you're returning early

+ (null !== $item) &&
+ (false !== $item) &&
+ (true !== $item) &&
+ in_array((string)$item, array('false', 'f', 'n', 'no', 'off'))

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 14, 2014

Member

Do the uppercase versions of these strings also act as false on pgsql? Additionally, the string cast is not needed if you pass true as the third parameter for in_array

@Ocramius

Ocramius Feb 14, 2014

Member

Do the uppercase versions of these strings also act as false on pgsql? Additionally, the string cast is not needed if you pass true as the third parameter for in_array

birko added some commits Feb 15, 2014

Update PostgreSqlPlatform.php
updated convert function  to include uppercased values
Update AbstractPostgreSqlPlatformTestCase.php
Updated test for uppercased values
@birko

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 15, 2014

Contributor

@Ocramius You were right I forgot to think about uppercased values

Contributor

birko commented Feb 15, 2014

@Ocramius You were right I forgot to think about uppercased values

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 15, 2014

Member

@deeky666 think another method in the abstract platform is ok? Otherwise, looks good to me.

Could need squashing of the commits, but I can do that before someone merges

Member

Ocramius commented Feb 15, 2014

@deeky666 think another method in the abstract platform is ok? Otherwise, looks good to me.

Could need squashing of the commits, but I can do that before someone merges

@birko

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 15, 2014

Contributor

@Ocramius Sorry for too much commints. I had problems cloning the repo into my PC so most of the changes I made here via Github edit function

Contributor

birko commented Feb 15, 2014

@Ocramius Sorry for too much commints. I had problems cloning the repo into my PC so most of the changes I made here via Github edit function

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Feb 15, 2014

Member

@birko that can be dealt with, no problem

Member

Ocramius commented Feb 15, 2014

@birko that can be dealt with, no problem

+ */
+ public function convertFromBoolean($item)
+ {
+ return (null === $item) ? null : (bool) $item;

This comment has been minimized.

Show comment Hide comment
@deeky666

deeky666 Feb 15, 2014

Member

Please use the short ternary expression: return null === $item ?: (bool) $item

@deeky666

deeky666 Feb 15, 2014

Member

Please use the short ternary expression: return null === $item ?: (bool) $item

+ */
+ public function convertFromBoolean($item)
+ {
+ if (

This comment has been minimized.

Show comment Hide comment
@deeky666

deeky666 Feb 15, 2014

Member

Please don't add a newline here after the parenthesis. This is against the PSR-2 coding standard.

@deeky666

deeky666 Feb 15, 2014

Member

Please don't add a newline here after the parenthesis. This is against the PSR-2 coding standard.

@deeky666

This comment has been minimized.

Show comment Hide comment
@deeky666

deeky666 Feb 15, 2014

Member

@birko Can you please give more details about the issue here? I can see that PostgreSQL allows different boolean values but I don't understand why we need to check for those when retrieving them from the database. AFAIK pdo_pgsql already converts them internally to the proper PHP boolean type. I did some quick tests about this and my assumption seems to be true. Is there another issue I am missing here?

Member

deeky666 commented Feb 15, 2014

@birko Can you please give more details about the issue here? I can see that PostgreSQL allows different boolean values but I don't understand why we need to check for those when retrieving them from the database. AFAIK pdo_pgsql already converts them internally to the proper PHP boolean type. I did some quick tests about this and my assumption seems to be true. Is there another issue I am missing here?

@birko

This comment has been minimized.

Show comment Hide comment
@birko

birko Feb 17, 2014

Contributor

@deeky666:
The main reason I created this pull request was when I wanted to solve this issue for my project https://github.com/l3pp4rd/DoctrineExtensions/issues/980

After a bit self brainstorming I thought it would be better if there was support for all Boolean literals (http://www.postgresql.org/docs/9.3/static/datatype-boolean.html), in case someone would like to use DBAL to parse database data that come not directly from database but in a kind of CSV export. (Like export to csv from pgadmin.)

And since in the Doctrine\DBAL\Types\BooleanType::convertToPHPValue was the $platform parameter I used it to make it more universal too.

Contributor

birko commented Feb 17, 2014

@deeky666:
The main reason I created this pull request was when I wanted to solve this issue for my project https://github.com/l3pp4rd/DoctrineExtensions/issues/980

After a bit self brainstorming I thought it would be better if there was support for all Boolean literals (http://www.postgresql.org/docs/9.3/static/datatype-boolean.html), in case someone would like to use DBAL to parse database data that come not directly from database but in a kind of CSV export. (Like export to csv from pgadmin.)

And since in the Doctrine\DBAL\Types\BooleanType::convertToPHPValue was the $platform parameter I used it to make it more universal too.

birko added some commits Feb 21, 2014

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.
@deeky666

This comment has been minimized.

Show comment Hide comment
@deeky666

deeky666 Feb 24, 2014

@birko You are right. My suggestion was wrong. Sorry for that.

@birko You are right. My suggestion was wrong. Sorry for that.

+ {
+ if ((null !== $item) &&
+ (false !== $item) &&
+ (true !== $item) &&

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 4, 2014

Member

All these cases can be removed, because they will not in be in the array

@stof

stof Mar 4, 2014

Member

All these cases can be removed, because they will not in be in the array

This comment has been minimized.

Show comment Hide comment
@birko

birko Mar 5, 2014

Contributor

@stof I added this additional conditions after one of previous hints here, That I should add them there to avoid unnecessary in_array check

@birko

birko Mar 5, 2014

Contributor

@stof I added this additional conditions after one of previous hints here, That I should add them there to avoid unnecessary in_array check

This comment has been minimized.

Show comment Hide comment
@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

It avoids unnessecary in_array() checks but adds new separate checks in the if statement and makes the code more complicated which reduces readability.

@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

It avoids unnessecary in_array() checks but adds new separate checks in the if statement and makes the code more complicated which reduces readability.

+ *
+ * @param mixed $item
+ *
+ * @return bool

This comment has been minimized.

Show comment Hide comment
@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

Please add the null as return value also, so: @return bool|null

@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

Please add the null as return value also, so: @return bool|null

+ if ((null !== $item) &&
+ (false !== $item) &&
+ (true !== $item) &&
+ in_array(strtolower($item), array('false', 'f', 'n', 'no', 'off'), true)

This comment has been minimized.

Show comment Hide comment
@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

I think '0' is missing, or does that not occur in Postgres?

@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

I think '0' is missing, or does that not occur in Postgres?

This comment has been minimized.

Show comment Hide comment
@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

Ok just noticed '0' is probably catched by the parent method doing some type juggling. Since your tests test for it.

@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

Ok just noticed '0' is probably catched by the parent method doing some type juggling. Since your tests test for it.

This comment has been minimized.

Show comment Hide comment
@birko

birko Jun 30, 2014

Contributor

yes. since it is the default common behaviuour :)

@birko

birko Jun 30, 2014

Contributor

yes. since it is the default common behaviuour :)

@@ -49,7 +49,7 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform)
*/
public function convertToPHPValue($value, AbstractPlatform $platform)
{
- return (null === $value) ? null : (bool) $value;
+ return $platform->convertFromBoolean($value);

This comment has been minimized.

Show comment Hide comment
@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

+1, when the to database conversion is platform specific, the to php version should also be platform specific.

@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

+1, when the to database conversion is platform specific, the to php version should also be platform specific.

+ {
+ $platform = $this->createPlatform();
+
+ $this->assertFalse($platform->convertFromBoolean(false));

This comment has been minimized.

Show comment Hide comment
@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

Nothing wrong with the test, but just wanted to say you could also have used data provider for this to reduce boilerplate

@lucasvanlierop

lucasvanlierop Jun 26, 2014

Contributor

Nothing wrong with the test, but just wanted to say you could also have used data provider for this to reduce boilerplate

birko added some commits Jun 30, 2014

updated return annotation
Updated from @return bool  -> @return bool|null since it returns null too
@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Jun 30, 2014

Member

Merged in #625

Member

Ocramius commented Jun 30, 2014

Merged in #625

@Ocramius Ocramius closed this Jun 30, 2014

@Ocramius Ocramius self-assigned this Jun 30, 2014

@Ocramius Ocramius referenced this pull request in zendframework/zendframework Mar 2, 2015

Closed

PDO, PostgreSQL and bool #7284

@GeeH GeeH referenced this pull request in zendframework/zend-db Jun 28, 2016

Open

PDO, PostgreSQL and bool #134

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