pdo_mysql: Fix boolean parameter binds failing when not using emulation. #3921

camporter · 2019-03-06T06:35:56Z

Attempts to fix bug 38546.

nikic · 2019-03-06T08:24:28Z

Could you please add a test for this change?

camporter · 2019-03-06T18:28:57Z

There seems to be a disagreement with the test for Bug 44707 about how to handle a difference in data types for booleans.

…ving boolean parameter behavior.

camporter · 2019-03-06T18:40:38Z

The last commit changes the test for that bug, because not modifying the variable that the boolean came from and enforcing boolean parameters seem to be separate concerns.

nikic · 2019-03-19T09:29:38Z

ext/pdo_mysql/mysql_statement.c

@@ -552,6 +552,8 @@ static int pdo_mysql_stmt_param_hook(pdo_stmt_t *stmt, struct pdo_bound_param_da
 						mysqlnd_stmt_bind_one_param(S->stmt, param->paramno, parameter, MYSQL_TYPE_VAR_STRING);
 						break;
 					case IS_LONG:
+					case IS_TRUE:
+					case IS_FALSE:


It seems like this is okay due to the code in https://github.com/php/php-src/blob/master/ext/mysqlnd/mysqlnd_ps_codec.c#L615. This will end up casting the booleans to double and then to integer, ending up with the expected value. Might be good to implement that more explicitly, as right now it seems like this working is a lucky accident rather than how this API was intended to be used.

I'd also appreciate someone who understands MySQL take a look at this. Maybe @johannes?

@nikic Disclaimer, I don't know a lot about C but do know a bit about MySQL.

I would not expect it to be casted to a double and then to an integer. What I would expect is that it is casted to an integer directly.

As per MySQL documentation the BOOL, BOOLEAN field is just a synonym for a TINYINT(1) field so I would expect this field type to also be used in the php mysql client. If I'm right there is already a type for this:
https://github.com/php/php-src/blob/master/ext/mysqlnd/mysqlnd_ps_codec.c#L405

Documentation of MySQL Bool type: https://dev.mysql.com/doc/refman/8.0/en/numeric-type-overview.html

So perhaps it would be better to use MYSQL_TYPE_TINY for IS_TRUE and IS_FALSE instead?

The cast to double is just a simple trick to see if the value might be bigger than maxint. Imagine you submit a large value on a platform where long is 32 bit, or an extremely huge value into an unsigned 64bit field. So there is no problem with applying this patch.

The whole type juggling might certainly be optimized ... but having a good test for this case is a good step towards making a refactoring there more possible with less risk of breaking anything :-)

@camporter @johannes In that case I would still use the MYSQL_TYPE_TINY type but make a expicit implementation that casts the value to double and then to int and not piggyback on the LONG, LONGLONG type functionality for the reason that they can to be separately maintained IMO.

Trying to use MYSQL_TYPE_TINY for IS_TRUE and IS_FALSE results in NULL values being sent to mysql. In the test on this PR:

Fatal error: Uncaught PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'some_bool_1' cannot be null in /home/cmporter/php-src/test.php:75

I'm possibly misunderstanding what the suggested change is.

@camporter most likely you will need to add supporting code to the mysqlnd_stmt_execute_prepare_param_types method to cast the booleans to double and then to integer when the type is MYSQL_TYPE_TINY .

In mysql_statement.c it would be something like the following:

case IS_TRUE: case IS_FALSE: mysqlnd_stmt_bind_one_param(S->stmt, param->paramno, parameter, MYSQL_TYPE_TINY); break;

@nikic I think that is what you mean with making it explicit, or did you mean something else? Or would you rather see a MYSQL_TYPE_BOOL added?

For tiny the hack with checking double first isn't needed. A tiny integer will in all cases fit into a long (PHP's int type) The double hack is just just to see whether the value is bigger than maxint in which case it is passed along as string (which is the only generic way to transport large values without loosing precision until PHP has a built-in bigint support)

Alright, so adding

case IS_TRUE: case IS_FALSE: mysqlnd_stmt_bind_one_param(S->stmt, param->paramno, parameter, MYSQL_TYPE_TINY); break;

requires mysqlnd_stmt_execute_store_param_values to handle MYSQL_TYPE_TINY specifically now, otherwise TRUE/FALSE values become NULL (default value stored). Storing 4 bytes the same as a long appears to work, but seems incorrect. Should it be storing 1 byte, and how would we go about handling that correctly? eg:

case MYSQL_TYPE_TINY: if (Z_TYPE_P(data) == IS_STRING) { goto send_string; } int4store(*p, Z_LVAL_P(data)); //int1store(?) (*p) += 4; // += 1 break;

We probably want to avoid making assumptions that only TRUE/FALSE values will use the TINY data type?

Is there any harm in having TRUE/FALSE be dealt with the same as LONG values?

So since it seems like we want to use MYSQL_TYPE_TINY so that TRUE/FALSE can use it from PDO, I made changes to support it in mysqlnd. Please check the new changes.

Pass MYSQL_TYPE_TINY if the bind value is TRUE/FALSE for pdo_mysql using mysqlnd. Store a single byte rather than 4 or 8.

nikic · 2019-04-23T10:23:11Z

Looks good to me. @johannes Do you also want to take a look?

rjd22 · 2019-04-24T11:37:24Z

@camporter looks really good, thx for the effort.

johannes · 2019-04-24T13:55:21Z

Looks good to me. Only small comment: If this is supported (supposed o be supported) by all drivers it might be a good idea to move the test to PDO's test suite as redirect test so it's run on all drivers.

camporter · 2019-04-24T19:46:30Z

I can try moving and running the test for PDO in general. Not sure how much will work, it will be a few days until I can complete checking that.

camporter · 2019-04-29T17:21:55Z

There seems to be quite a bit of variance between various drivers about how and whether they handle a boolean type, which doesn't exactly match what is being fixed on this specific pull request.

I'm up for additional changes to get consistency between the drivers as far as handling boolean values, but I'm not sure what the expected behavior is supposed to be under every supported driver.

Adding confusion is that boolean values fetched from (for example) mysql are returned as integers, instead of true/false specifically because they are represented as numbers. It's not obvious to me how much PDO can or should 'abstract' in various boolean type cases.

johannes · 2019-04-29T20:02:50Z

Result types we can't unify further - he database tells a type and that's all info we have (till we know table schemas and have an sql engine or user requests a specific type - which both are out of scope for PDO)

If writing a generic est is too hard then I wouldn't bother now but suggest to push the change.

camporter · 2019-06-06T18:29:20Z

I was not able to come up with a generic test that would make sense for more than the mysql driver in PDO due to driver and database differences.

nikic · 2019-06-07T07:50:36Z

Merged as 7d1aa75 into PHP 7.2 and up. Thanks a lot for working on this!

camporter added 2 commits March 6, 2019 00:33

pdo_mysql: Fix boolean parameter binds failing when not using emulation.

6753cba

Fix pointless copy-paste in first commit.

5af74be

KalleZ added Bug Quickfix Waiting on Author labels Mar 6, 2019

Add test for the fix.

03d5b78

Change test for bug 44707, which seems to weirdly enforce the unforgi…

905a547

…ving boolean parameter behavior.

KalleZ removed the Waiting on Author label Mar 7, 2019

nikic reviewed Mar 19, 2019

View reviewed changes

camporter added 2 commits April 20, 2019 19:21

Add MYSQL_TYPE_TINY bind handling to mysqlnd.

b53f5e1

Pass MYSQL_TYPE_TINY if the bind value is TRUE/FALSE for pdo_mysql using mysqlnd. Store a single byte rather than 4 or 8.

Remove else for consistency.

675b1aa

nikic self-assigned this Jun 6, 2019

nikic closed this Jun 7, 2019

pdo_mysql: Fix boolean parameter binds failing when not using emulation. #3921

pdo_mysql: Fix boolean parameter binds failing when not using emulation. #3921

Uh oh!

Conversation

camporter commented Mar 6, 2019 • edited by nikic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Mar 6, 2019

Uh oh!

camporter commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camporter commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjd22 Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjd22 Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Apr 23, 2019

Uh oh!

rjd22 commented Apr 24, 2019

Uh oh!

johannes commented Apr 24, 2019

Uh oh!

camporter commented Apr 24, 2019

Uh oh!

camporter commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johannes commented Apr 29, 2019

Uh oh!

camporter commented Jun 6, 2019

Uh oh!

nikic commented Jun 7, 2019

Uh oh!

Uh oh!

camporter commented Mar 6, 2019 •

edited by nikic

Loading

camporter commented Mar 6, 2019 •

edited

Loading

camporter commented Mar 6, 2019 •

edited

Loading

rjd22 Mar 21, 2019 •

edited

Loading

rjd22 Mar 23, 2019 •

edited

Loading

camporter commented Apr 29, 2019 •

edited

Loading