Add custom boolean value casting #242
Conversation
|
I'm glad finding this. It's a must-have, at least for PgSQL. Thanks! Why above commits are not in nightly build? |
|
@jpfuentes2 take a look at this PR when you have a chance. Other ORMs I have worked with behave similarly, delegating the conversion of types to the adapters. Not every vendor accepts the same values for booleans, dates, etc. |
|
I'm in favor of this strategy. That being said, do you think you could fix the indentation formatting? It's not in harmony with the existing code. Also, would you add a specific test to Thanks |
|
@jpfuentes2 sorry to have taken so long to get back on this, I think it should be good if it looks good to you. Thanks for looking at this PR! I've fixed the formatting and added a specific test to Thanks again for checking this out, you have a great project here - glad to be able to contribute back to it. I have also put in another PR #243 for a feature that I've found very handy in handing virtual attributes if you care to take a look. |
|
I personally like this one 👍 |
|
Absolutely essential for PgSQL support. 👍 |
| @@ -30,6 +31,8 @@ class Column | |||
| 'date' => self::DATE, | |||
| 'time' => self::TIME, | |||
|
|
|||
| 'boolean' => self::BOOLEAN, | |||
There was a problem hiding this comment.
What whitespace are you referring to?
There was a problem hiding this comment.
It's an associative array and all other keys => values are on subsequent lines except for your addition. Perhaps this was a previous issue, but could you tighten it up?
Thanks!
There was a problem hiding this comment.
This probably is because the array is somewhat divided into types, and boolean is its own 'type', see here
There was a problem hiding this comment.
No worries. It does look a bit odd in the diff!
There was a problem hiding this comment.
Do you want me to remove those blank lines separating the different types? I thought it sounded like we'd leave it as is, but maybe I misunderstood. Are you waiting on me for anything to tidy up to merge this PR?
|
@leightonshank Could you rebase your branch on master? |
The default PHP boolean-to-string casting does not work for some database connection types (e.g. PostgreSQL). Added a BOOLEAN type to the Column class, and then the cast() method delegates to the a new boolean_to_string() method in the Connection class. By default the boolean_to_string() method returns the standard PHP string cast for the boolean, retaining the original behavior. This method can be overridden in adapter classes to provides custom casting of boolean values. Added an overridden boolean_to_string() method into the adapters/PgsqlAdapter.php class which casts the boolean values false and true to the strings "0" and "1" respectively. These string values are then recognized by the Postgres database as boolean values, as opposed to the values "" and "1" generated by the standard PHP cast.
Updated the ColumnTest::assert_cast() to test that boolean values are being casted using the Connection boolean_to_string() method. Added a PostgreSQL specific test to ensure that boolean values are being casted as the correct string value.
Modified the boolean_to_string() method in the PgsqlAdapter class to properly convert values that PHP considers a boolean, as well as what PostgreSQL considers a boolean value. E.g., PostgreSQL considers the values 'f', 'false', 'n', 'no', and 'off' as valid values for `false`, but PHP does not. The original implementation of this method would return `true` for one of these values which is incorrect for a PostgreSQL database. Also modified the base Connection class boolean_to_string() method to first cast its `$value` parameter as a boolean before casting it to a string. This will account for PHP's loose type-casting and make sure the method returns a consistent casted value.
While the previous version was explicit and descriptive, it wasn't that elegant. This is much nicer.
|
@koenpunt Sorry for the delay on this. This branch has been rebased on master. Another thing that I just noticed -- The Travis CI build failed so I went to investigate why. Travis is reporting an error: After investigating, it looks like Travis CI updated I ran the tests on my local machine with phpunit v3 and they passed. Have you guys seen this issue with the updated version of PHPUnit on Travis? Any plans to address it? |
|
Don't know about an updated PHPUnit but we can probably lock it to a specific version. Currently on mobile so I'm not able to investigate further |
|
Thanks to @koenpunt this should be resolved. Can we get master merged to rebuild with the changes? |
|
Yup I'll merge the master in this afternoon for ya. Sent from my iPhone
|
|
Alright, master is merged in and the build is green. Nice fix @koenpunt -- I had just started looking at doing the same thing Friday afternoon 😄 |
I ran into an issue where the default PHP casting of false boolean values (PHP casts false to an empty string) didn't work with a PostgreSQL database which doesn't accept an empty string for a boolean value.
To get around this, I added a new BOOLEAN column type, updated the
cast()method in theColumnclass to delegate to theConnectionclass for converting a boolean to string values. By default it still uses the standard PHP casting (i.e.(string)$value), but can be overridden in an adapter connection class for custom handling of boolean conversion.