php / php-src

The PHP Interpreter
https://www.php.net
Other
38.13k stars 7.74k forks source link

PDO execute not correctly binding booleans #9644

Open 8ctopus opened 2 years ago

8ctopus commented 2 years ago

Description

The following code:

<?php

$db = new PDO(...);

$sql = <<<SQL
    CREATE TABLE test (
        `id` INT NOT NULL AUTO_INCREMENT PRIMARY KEY,
        `birthday` DATE NOT NULL,
        `name` VARCHAR(40) NOT NULL,
        `salary` INT NOT NULL,
        `boss` BIT NOT NULL
    ) ENGINE=InnoDB DEFAULT CHARSET=utf8
SQL;

$query = $db->prepare($sql);
$query->execute();

$sql = <<<SQL
    INSERT INTO test
    (birthday, name, salary, boss)
    VALUES (:birthday, :name, :salary, :boss)
SQL;

$query = $db->prepare($sql);

$staff = [
    [
        'birthday' => (new DateTime('1995-05-01'))->format('Y-m-d'),
        'name' => 'Sharon',
        'salary' => '200',
        'boss' => TRUE,
    ],
];

foreach ($staff as $member) {
/* works
    $query->bindValue('birthday', $member['birthday'], PDO::PARAM_STR);
    $query->bindValue('name', $member['name'], PDO::PARAM_STR);
    $query->bindValue('salary', $member['salary'], PDO::PARAM_INT);
    $query->bindValue('boss', $member['boss'], PDO::PARAM_BOOL);
    $query->execute();
*/
    // does not work
    $query->execute($member);
}

Resulted in this output:

SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'boss' at row 1

But I expected execute() not to throw an error.

I clearly see the problem in the source as the parameter is always set to be a string on line 424:

https://github.com/php/php-src/blob/615b8006c42715b5ea9ec61f9368582eeef8467f/ext/pdo/pdo_stmt.c#L411-L425

Can't the execute() code be improved to check for the param type like it is done in bindValue()?

https://github.com/php/php-src/blob/615b8006c42715b5ea9ec61f9368582eeef8467f/ext/pdo/pdo_stmt.c#L1465-L1475

PHP Version

8.2.0 RC3

Operating System

Alpine Linux

devnexen commented 2 years ago

Sounds like a fair point cc @Girgias

cmb69 commented 2 years ago

From the docs:

All values are treated as PDO::PARAM_STR.

So not a bug, but rather a feature request. Still, only treating values of type bool as PDO::PARAM_BOOL and all others as PDO::PARAM_STR would be more confusing than as it's now. However, how to map other types is not necessarily clear.

8ctopus commented 2 years ago

@cmb69 Sorry I overlooked the mention in the documentation. I see 2 options:

IMO it would be convenient for execute() not to convert all types to string but keep the native type, however I imagine it can cause problems when migrating existing code to newer php versions (unless the new behavior gets implemented in a new method name).

8ctopus commented 2 years ago

@cmb69 I've given the problem some thought over the weekend and here's what I came up with.

My understanding is that it is not possible to pass a boolean value to execute() without the SQL query failing therefore it should qualify as bug or otherwise the documentation should be updated mentioning that query parameters containing booleans need to be bound manually.

What do you think?

cmb69 commented 2 years ago

My understanding is that it is not possible to pass a boolean value to execute() without the SQL query failing […]

I'm not sure whether the query fails for all drivers. Maybe we should have a test case for that so we can run it in CI which covers all (?) bundled drivers at least.

otherwise the documentation should be updated mentioning that query parameters containing booleans need to be bound manually.

Is this only an issue wrt. to bools? Would all drivers handle other types (e.g. int) correctly?