Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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:
```
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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! -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
I couldn't express myself well in English, I'm sure my words were bad. sorry. Saki -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 On Sun, Nov 5, 2023, 10:10 Saki Takamachi wrote: > 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 > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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_` 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/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
Hi, internals.
> As shown in the following issue, the behavior of `PDO::PARAM_` 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:
```php
$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 :
>
> Hi, Ayesh,
>
> I forgot to tell you one important thing.
>
> Binding of parameters is not actually done in the `bind`method, 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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
Hi Hans, Thank you, I will discuss it and improve it in a way that everyone is satisfied with. Saki -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
I had no idea PDO's PARAM_INT and PARAM_BOOL was so buggy, good catch! On Sat, Nov 4, 2023, 07:59 Saki Takamachi wrote: > Hi internals, > > As shown in the following issue, the behavior of `PDO::PARAM_` 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 >
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
Hi, Ayesh, I forgot to tell you one important thing. Binding of parameters is not actually done in the `bind`method, 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
> > Hi internals, > > As shown in the following issue, the behavior of `PDO::PARAM_` 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. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
[PHP-DEV] Fix the inconsistent behavior of PDO::PARAM_XXX
Hi internals, As shown in the following issue, the behavior of `PDO::PARAM_` 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
