Fix the inconsistent behavior of PDO::PARAM_XXX

6 months ago by Saki Takamachi — view source

unread

Hi internals,

As shown in the following issue, the behavior of PDO::PARAM_XXXX is inconsistent and I would like to fix this.
https://github.com/php/php-src/issues/12603

First, I tried fixed pdo_pgsql.
https://github.com/php/php-src/pull/12605

Eventually I plan to make similar changes to all bundled pdos.

What do you think about these? If there is a reason for the current implementation that should not be changed, it would be very helpful if you could tell me why.

Also, if an RFC is required, it would be helpful if you could point it out as well. Personally, I don't think an RFC is necessary since this is some kind of bug fix. However, I believe the target should be the master branch as it changes the user experience somewhat.

[*] I'm not thinking about LOB here, but once I leave them with their existing behavior, I change the behavior of other types. Because LOB have complex behavior, the scope becomes too large. After making this change, I will test again and post another issue if necessary.

[*] I would like the type of PARAM_BOOL to be exactly bool, but this would also require changing the behavior of the emulator, and I would not be able to issue a PR for each driver, making the scope too large. For this as well, I will just align it to int(1) or string(1) 't', etc., and once these changes are completed, I will verify it and post an issue.

[*] odbc etc. are not compatible with emulation in the first place. There are no plans to change this in this context.

Regards.

Saki

6 months ago by Ayesh Karunaratne — view source

unread

Hi internals,

As shown in the following issue, the behavior of PDO::PARAM_XXXX is inconsistent and I would like to fix this.
https://github.com/php/php-src/issues/12603

First, I tried fixed pdo_pgsql.
https://github.com/php/php-src/pull/12605

Eventually I plan to make similar changes to all bundled pdos.

What do you think about these? If there is a reason for the current implementation that should not be changed, it would be very helpful if you could tell me why.

Also, if an RFC is required, it would be helpful if you could point it out as well. Personally, I don't think an RFC is necessary since this is some kind of bug fix. However, I believe the target should be the master branch as it changes the user experience somewhat.

[*] I'm not thinking about LOB here, but once I leave them with their existing behavior, I change the behavior of other types. Because LOB have complex behavior, the scope becomes too large. After making this change, I will test again and post another issue if necessary.

[*] I would like the type of PARAM_BOOL to be exactly bool, but this would also require changing the behavior of the emulator, and I would not be able to issue a PR for each driver, making the scope too large. For this as well, I will just align it to int(1) or string(1) 't', etc., and once these changes are completed, I will verify it and post an issue.

[*] odbc etc. are not compatible with emulation in the first place. There are no plans to change this in this context.

Regards.

Saki

I also commented on the PR, but in the interest of keeping the
conversation in the mailing list, I'll summarize my thoughts here as
well.

I don't think we should change the existing behavior. It may be an
xkcd/1172 case, but these kinds of cases are very hard to spot from
static analyzers, and are often only surfaced in production only if
someone spent enough time to dig deep. It's not helping when certain
database softwares try to be forgiving and sloppy with types (looking
at you, MySQL).

In the PR, I proposed to add new methods (bindStringValue,
bingBoolValue, etc) as a non-BC approach, that also rely on the
existing type system to communicate the types and enforce them. Saki
said that it might not be possible to introduce 10+ new methods to
PDOStatement, to which I also agree. However, maybe we can
conservatively add the basic four types (BOOL/NULL/INT/STR) as a
start.

6 months ago by Saki Takamachi — view source

unread

Hi, Ayesh,

I forgot to tell you one important thing.

Binding of parameters is not actually done in the bindXXXXmethod, but in the execute() method. Therefore, when preparing a new method, a slightly larger mechanism is required to determine which method the parameter was set through.

Regards.

Saki

6 months ago by Kentaro Takeda via internals — view source

unread

Hi, internals.

As shown in the following issue, the behavior of PDO::PARAM_XXXX is
inconsistent and I would like to fix this.
https://github.com/php/php-src/issues/12603

I consider the current behavior a bug.
And some of them contain behaviors that are very confusing to users.

It may be an xkcd/1172 case, but these kinds of cases are very hard
to spot from static analyzers, and are often only surfaced
in production only if someone spent enough time to dig deep.

As previously discussed by Ayesh, assessing
the users' impact of a fix for this issue might be a little challenging,

but I think it's important to address it to prevent confusion.

Taking PostgreSQL as an example, let's consider the following PHP code:

$pdo = new PDO('pgsql:');
$pdo->exec('create temp table t(boolean_column bool, text_column text)');

$stmt = $pdo->prepare('insert into t values (:v1, :v2)');
$stmt->bindValue(':v1', false, PDO::PARAM_BOOL);
$stmt->bindValue(':v2', false, PDO::PARAM_BOOL);
$stmt->execute();

$result = $pdo->query('select * from t');
var_export($result->fetchAll(PDO::FETCH_ASSOC));

The result should look like this:

array (
   0 =>
   array (
     'boolean_column' => false,
     'text_column' => 'f', // HERE!!!
   ),
)

Even if we insert false, it will change to 'f' when we select it.

This is fundamentally a user mistake. However, the big problem in this case is
that the value 'f' after the change is truthy. Who could have predicted
that even though you entered false, it ended up being true ?

I've seen a lot of pointless code to deal with problems
like this where types are not uniformly formatted.

As Saki says, we need to think carefully about whether
we can accurately treat this as bool.
However, regarding the above problem:

Fix truthy / falsy such as int(1) / int(0) to a form that
can be correctly determined.
Make this behavior consistent across as many drivers as possible.

Even this small fix would eliminate much of the confusion that is
currently occurring.
(Of course, it should ultimately be treated as bool.)

I look forward to a resolution that will be satisfactory to all.
Thank you for your attention.

2023年11月4日(土) 17:56 Saki Takamachi saki@sakiot.com:

Hi, Ayesh,

I forgot to tell you one important thing.

Binding of parameters is not actually done in the bindXXXXmethod, but in the execute() method. Therefore, when preparing a new method, a slightly larger mechanism is required to determine which method the parameter was set through.

Regards.

Saki

--

To unsubscribe, visit: https://www.php.net/unsub.php

6 months ago by Saki Takamachi — view source

unread

Hi Kentaro,

The case you presented certainly confuses us very much. Although it is outside the scope of this discussion, it certainly seems like it would be better to improve it from a safety perspective.

BOOL probably has the most problems; for example, when you pass str, the boolean value will be false only in sqlite. This happens because str is converted to long and then to bool.

At least for this kind of behavior, I don't think it's necessary to maintain compatibility.

Regards.

Saki

6 months ago by Hans Henrik Bergan — view source

unread

I had no idea PDO's PARAM_INT and PARAM_BOOL was so buggy, good catch!

Hi internals,

As shown in the following issue, the behavior of PDO::PARAM_XXXX is
inconsistent and I would like to fix this.
https://github.com/php/php-src/issues/12603

First, I tried fixed pdo_pgsql.
https://github.com/php/php-src/pull/12605

Eventually I plan to make similar changes to all bundled pdos.

What do you think about these? If there is a reason for the current
implementation that should not be changed, it would be very helpful if you
could tell me why.

Also, if an RFC is required, it would be helpful if you could point it out
as well. Personally, I don't think an RFC is necessary since this is some
kind of bug fix. However, I believe the target should be the master branch
as it changes the user experience somewhat.

[*] I'm not thinking about LOB here, but once I leave them with their
existing behavior, I change the behavior of other types. Because LOB have
complex behavior, the scope becomes too large. After making this change, I
will test again and post another issue if necessary.

[*] I would like the type of PARAM_BOOL to be exactly bool, but this would
also require changing the behavior of the emulator, and I would not be able
to issue a PR for each driver, making the scope too large. For this as
well, I will just align it to int(1) or string(1) 't', etc., and once
these changes are completed, I will verify it and post an issue.

[*] odbc etc. are not compatible with emulation in the first place. There
are no plans to change this in this context.

Regards.

Saki

6 months ago by Saki Takamachi — view source

unread

Hi Hans,

Thank you, I will discuss it and improve it in a way that everyone is satisfied with.

Saki

6 months ago by Saki Takamachi — view source

unread

Hi,

To think more deeply about this issue, I investigated various cases in major databases. It's too big to write in an email, so I posted it in the comments of the issue.

https://github.com/php/php-src/issues/12603#issuecomment-1793679776

Regards.

Saki

6 months ago by Hans Henrik Bergan — view source

unread

I think it'd be a good idea if they used FILTER_VALIDATE_BOOLEAN and
FILTER_VALIDATE_INTEGER type logic, with an error if conversion fails..

I wonder if PDO::PARAM_BOOL_OR_NULL would be worthwhile

Hi,

To think more deeply about this issue, I investigated various cases in
major databases. It's too big to write in an email, so I posted it in the
comments of the issue.

https://github.com/php/php-src/issues/12603#issuecomment-1793679776

Regards.

Saki

To unsubscribe, visit: https://www.php.net/unsub.php

5 months ago by Saki Takamachi — view source

unread

Hi Hans,

I think it'd be a good idea if they used FILTER_VALIDATE_BOOLEAN and FILTER_VALIDATE_INTEGER type logic, with an error if conversion fails..

This can be difficult. Forcing an error is highly likely to destroy the existing user environment.

I wonder if PDO::PARAM_BOOL_OR_NULL would be worthwhile

That's what I thought at first, but I think it might be a good idea to leave it as a fallback, especially for NULL. In fact, there have been proposals to deprecate PARAM_NULL in the past, but none have made it to the voting phase. I have not looked into it in detail yet, but I suspect that during the discussion stage, they may have come to the conclusion that it should not be abolished.

Regards.

Saki

6 months ago by Matteo Beccati — view source

unread

Hi Saki,

Il 04/11/2023 07:59, Saki Takamachi ha scritto:

Hi internals,

As shown in the following issue, the behavior of PDO::PARAM_XXXX is inconsistent and I would like to fix this.
https://github.com/php/php-src/issues/12603

First, I tried fixed pdo_pgsql.
https://github.com/php/php-src/pull/12605

Eventually I plan to make similar changes to all bundled pdos.

What do you think about these? If there is a reason for the current implementation that should not be changed, it would be very helpful if you could tell me why.

I'm sure there are several bugs w/ types in PDO.

Perhaps some can be fixed, but I would be very careful touching that part.

The last time I tried to fix the PDO_PARAM_INT behaviour on pdo_pgsql I
broke Laravel and surely many other projects.

I'm afraid that most libraries and projects have now worked around some
of the bugs and trying to fix them is going to cause BC problems, or
generate a whole new series of bugs and incompatibilities.

See:

https://bugs.php.net/bug.php?id=80892
https://github.com/php/php-src/pull/6801

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

6 months ago by Saki Takamachi — view source

unread

Hi Matteo,

I'm sure there are several bugs w/ types in PDO.

Perhaps some can be fixed, but I would be very careful touching that part.

The last time I tried to fix the PDO_PARAM_INT behaviour on pdo_pgsql I broke Laravel and surely many other projects.

I'm afraid that most libraries and projects have now worked around some of the bugs and trying to fix them is going to cause BC problems, or generate a whole new series of bugs and incompatibilities.

I hate to be the person to tell you this, it is simply a problem caused by releasing broken code.

However admittedly, we may need a some more test patterns to compensate for not breaking functionality.

Regards.

Saki

6 months ago by Saki Takamachi — view source

unread

I should have said that the spec was broken, not the code. By that logic, my PR is probably broken as well. I did additional research on each DB to see if the specs were broken.

Saki

5 months ago by Kentaro Takeda via internals — view source

unread

Hi, Matteo,

The last time I tried to fix the PDO_PARAM_INT behavior on pdo_pgsql I broke Laravel and surely many other projects.
https://github.com/php/php-src/pull/6801

Thank you for providing an important example.

Regarding this past issue with PostgreSQL, it can be solved by
treating numbers larger than int4 as unknown (as is the case now)
rather than as int8 (as in previous attempts).

This approach aligns with PostgreSQL's capability to cast integer to
boolean but not smallint or bigint. This consideration is
crucial for ensuring interoperability.

postgres=# select 1::integer::boolean ; -- returns `t`
postgres=# select 1::smallint::boolean ; -- `ERROR: cannot cast type
smallint to boolean`
postgres=# select 1::bigint::boolean ; -- `ERROR: cannot cast type
bigint to boolean`

It is certainly not realistic to unify behavior across all databases.
Also, as in the example above, there are certainly places where each
database requires individual support.

I think it would be good if a solution could be found that would allow
for more correct behavior without breaking the behavior of existing
applications.

Your insights and contributions are interesting and invaluable. They
are much appreciated.
Best regards

5 months ago by Kentaro Takeda via internals — view source

unread

Hi, Matteo

Regarding this past issue with PostgreSQL, it can be solved by
treating numbers larger than int4 as unknown (as is the case now)
rather than as int8 (as in previous attempts).

I'm sorry, this was my mistake.

select 1::boolean ; -- returns `t`
select 1 = true ; -- `ERROR: operator does not exist: integer = boolean`

The conditions for casting and comparison were different.
PDO_PARAM_INT still needs to be passed as unknown to PostgreSQL.

Thank you!

5 months ago by Saki Takamachi — view source

unread

I couldn't express myself well in English, I'm sure my words were bad. sorry.

Saki

5 months ago by Saki Takamachi — view source

unread

Hi,

I think this is probably the same problem that Matteo ran into, but when using emulate mode, pgsql errors out with code like this:

&lt;?php
$db = new PDO(
    pgsql:/* dsn */,
    /* username */,
    /* password */,
    [
        PDO::ATTR_EMULATE_PREPARES => true,
    ],
);

$db->exec('CREATE TABLE test2 (val BOOLEAN)');

$stmt = $db->prepare('INSERT INTO test2 VALUES(:val)');
$stmt->bindValue(':val', 1, PDO::PARAM_INT);
$stmt->execute();

Fatal error: Uncaught PDOException: SQLSTATE[42804]: Datatype mismatch: 7 ERROR:  column "val" is of type boolean but expression is of type integer

It seems a little strange that whether an error occurs or not depends on whether you are in emulation mode or not.

Saki

5 months ago by Saki Takamachi — view source

unread

As Ayesh says, I'm starting to feel like I need to provide a completely new feature separately tbh…

The following issues are completely bugs and should at least be fixed.
https://github.com/php/php-src/issues/12581

Saki