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.

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

Boolean data type support, added in Firebird 3. Fixes #74462. #2522

Closed
wants to merge 2 commits into
base: master
from

Conversation

7 participants
@madorin
Contributor

madorin commented May 13, 2017

More info: https://bugs.php.net/bug.php?id=74462
Note: this require Firebird 3.0 deps

@madorin

This comment has been minimized.

Contributor

madorin commented May 13, 2017

Safe for merging in 7.x branches.

@KalleZ

This comment has been minimized.

Contributor

KalleZ commented May 13, 2017

Hi

If this requires the Firebird 3.0 deps, shouldn't it be #ifdef'd out? (Ps I did not read the entire code but your patch)

-K

@madorin

This comment has been minimized.

Contributor

madorin commented May 13, 2017

@KalleZ, I think we should just update FB deps. It is backward compatible.

@KalleZ

This comment has been minimized.

Contributor

KalleZ commented May 13, 2017

@madorin in that case, I'm all for it :)

@weltling

This comment has been minimized.

Contributor

weltling commented May 13, 2017

@KalleZ is right, that needs to be ifdef'd. The source can be linked with an arbitrary lib, so it needs to be compatible. As well, the test should care about it and skip if necessary.

Yet one question - what happens when the new datatype is sent to an older FB server? Were it possible to mitigate such a situation?

Thanks.

@weltling

This comment has been minimized.

Contributor

weltling commented May 15, 2017

The Firebird 3.0 deps are added to the staging series and can be fetched automatically with the new SDK, they're also automatically in use with Appveyor and snaps now.

Thanks.

@mariuz mariuz referenced this pull request May 16, 2017

Closed

Support to Firebird 3.0 #3

@edgardmessias

This comment has been minimized.

edgardmessias commented May 17, 2017

First, thank's @madorin.

Binding work only strings

$stmt = $pdo->prepare("SELECT * FROM TBOOL WHERE BVAL = :bool");

//WORK
$stmt->bindValue(":bool", 'true', PDO::PARAM_STR);
$stmt->bindValue(":bool", 'false', PDO::PARAM_STR);
$stmt->bindValue(":bool", 'true', PDO::PARAM_BOOL);
$stmt->bindValue(":bool", 'false', PDO::PARAM_BOOL);

//NOT WORK
$stmt->bindValue(":bool", true, PDO::PARAM_STR);
$stmt->bindValue(":bool", false, PDO::PARAM_STR);
$stmt->bindValue(":bool", true, PDO::PARAM_BOOL);
$stmt->bindValue(":bool", false, PDO::PARAM_BOOL);
$stmt->bindValue(":bool", 1, PDO::PARAM_INT);
$stmt->bindValue(":bool", 0, PDO::PARAM_INT);
$stmt->bindValue(":bool", 1, PDO::PARAM_BOOL);
$stmt->bindValue(":bool", 0, PDO::PARAM_BOOL);
@madorin

This comment has been minimized.

Contributor

madorin commented May 17, 2017

Hello Edgar,
Current implementation is for returned fields only and params you mentioned.
In my private branch I have also full support for bool params, but I'm trying to initiate a discussion in PHP dev groups for a more intelligent solution, more info here: http://news.php.net/php.internals/99052
P.S. by the way, you could do just $stmt->bindValue('bool', 'true'); STR is the default type, and omit ":".

@krakjoe krakjoe added the Bugfix label May 25, 2017

@krakjoe

This comment has been minimized.

Member

krakjoe commented Feb 8, 2018

What's the current status of this patch ?

@madorin

This comment has been minimized.

Contributor

madorin commented Feb 8, 2018

@krakjoe, it is safe to be merged.

I saw in meantime also dependencies (php-sdk) on windows have been updated to Firebird 3 and it is also default now on all major Linux distributions.

It implements boolean data type for fields, introduced in Firebird3.
For example, we may do "select bool_field from a_table" and it will return a true bool data type field :)

It still does not fully supports boolean type statement parameters as @edgardmessias pointed, but this is another story. Hope in near future will have some free time to implement also boolean parameters.

@mariuz

This comment has been minimized.

Contributor

mariuz commented Mar 29, 2018

@krakjoe Could you merge this ?

@LifeOrYou

This comment has been minimized.

LifeOrYou commented Jun 19, 2018

Nobody merge this ?

@madorin

This comment has been minimized.

Contributor

madorin commented Jun 20, 2018

@weltling, can You merge this please?

@weltling

This comment has been minimized.

Contributor

weltling commented Jun 20, 2018

@madorin thanks for the ping. It still looks like this part should be guarded. There are lots of LTS distribution users around, fe Jessie.

Thanks.

@madorin

This comment has been minimized.

Contributor

madorin commented Jun 20, 2018

Oki, yes, for linux distros it make sense to add some ifdefs. i saw for windows platform ibase.h was updated. I'll try to solve it during the weekend. Thanks!

@weltling

This comment has been minimized.

Contributor

weltling commented Jul 3, 2018

I've merged this PR as 78f23a6 and added #ifdefs myself.

Thanks.

@weltling weltling closed this Jul 3, 2018

@madorin

This comment has been minimized.

Contributor

madorin commented Jul 4, 2018

@weltling , Firebird community really appreciate Your work, and is sending You a big thanks! :)

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