Ensured parametrised queries respect data types

[Mr-Kaos]

If using Database::execute() with an array of values of varying types, particularly booleans or ints, they are encased in quotes, causing them to be interpreted as strings.
This would result in the following error:
SQL Error: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'BoolVal' at row 1 (0)

This was found by attempting to call a stored procedure that took a bit as a parameter, in which the boolean in PHP was passed as a string, violating the max length of the parameter.

Below is some sample code I used to replicate the issue before the fix:

MySQL Stored Procedure:

DELIMITER $$
$$
CREATE PROCEDURE DEBUG(
	IN String nvarchar(128),
	IN Nullable varchar(32),
	IN NumberVal INT,
	IN BoolVal bit)
BEGIN
	SELECT String, Nullable, NumberVal, BoolVal;
END
$$
DELIMITER ;

PHP Code:

<?php
$data['String'] = 'hello';
$data['Nullable'] = null;
$data['NumberVal'] = 21;
$data['BoolVal'] = true; // This causes an error when executed.

db->execute("CALL DEBUG(?,?,?,?)", array_values($data));

Note: I could not test for the same erroneous behaviour using named parameters, as I believe MySQL does not support them in stored procedures. I included the fix in that foreach loop anyway as it looks like it would encounter the same issue if used.

[Mr-Kaos]

Just realised that if a numeric string is given, it will always ascertain as is_numeric, which will likely cause issues for string-based parameters that could take a numeric string.

This will need to be corrected.

[Mr-Kaos]

Just realised that if a numeric string is given, it will always ascertain as is_numeric, which will likely cause issues for string-based parameters that could take a numeric string.

Did some tests, and it seems MySQL does not have a problem with numeric values being passed as strings into a procedure. However, I am not sure how this will fare in other database languages.

At the very least, this ensures that bit-typed parameters work:

foreach ($this->positionalParams as $value) {
    switch (true) {
        case is_bool($value):
            $pdoStatement->bindValue($i, $value, PDO::PARAM_BOOL);
            break;
        default:
            $pdoStatement->bindValue($i, $value, PDO::PARAM_STR);
    }
    $i++;
}

foreach ($this->namedParams as $name => $value) {
    switch (true) {
        case is_bool($value):
            $pdoStatement->bindValue($name, $value, PDO::PARAM_BOOL);
            break;
        default:
            $pdoStatement->bindValue($name, $value, PDO::PARAM_STR);
    }
}

[joshmcrae]

Looks good if we make a slight modification.

I'm thinking this will need to be a breaking change.

[joshmcrae]

is_numeric will return true if:

• Value is a string containing a numeric value
• Value is an int
• Value is a float

Since PDO only has a param type for ints specifically and we want to allow int-only values in actual strings, is_int should be used here instead. This means floats and strings containing numeric values are still bound as strings.

[Mr-Kaos]

I thought I had committed that last commit many months ago, but I had clearly forgotten.

However, I have found that my last commit here has a pretty big bug that prevents INSERTs from executing in certain scenarios.
I was playing around with the save() and insert() functions and found that adding the switch case for the data type causes other issues when a null value is bound in an INSERT, MySQL throws an error stating:

SQLSTATE[HY093]: Invalid parameter number: parameter was not defined

For example, running:

$conn->table('Components')->save([
    'ComponentName' => 'Cool Name',
    'ComponentDescription' => "sample description",
    'ComponentType' => 12,
    'MasterRevision' => null
]);

Will produce the following query:

INSERT INTO `Components`
    (`ComponentName`, `ComponentDescription`, `ComponentType`, `MasterRevision`)
VALUES
    ('Cool Name', 'sample description', '12', ':MasterRevision')

This does not occur if the "MasterRevision" column is bound to a string using $pdoStatement->bindValue($name, $value, PDO::PARAM_STR);.

I am wondering if perhaps removing the null value case in the switch block would be acceptable?

switch (true) {
    case is_int($value):
        $pdoStatement->bindValue($i, $value, PDO::PARAM_INT);
        break;
    case is_bool($value):
        $pdoStatement->bindValue($i, $value, PDO::PARAM_BOOL);
        break;
    default:
        $pdoStatement->bindValue($i, $value, PDO::PARAM_STR);
}

[Mr-Kaos]

I would like to add some unit tests to ensure this doesn't cause any other affects, but I cannot get the docker image to connect to the created mysql container.
Unsure if there's an error with my docker installation, or in the mysql image used, but I cannot get the unit tests to run without receiving a SQLSTATE[HY000] [2002] Connection refused error for all tests. I don't think I had these errors when I added the tests for the mssql driver, but I could be misremembering.

[joshmcrae]

@Mr-Kaos I'm getting no errors when running the test suite from the latest master.

You may want to do a clean rebuild of the Docker Compose stack and then try again (making sure you're running tests via composer test and not just running PHPUnit from your host machine).

docker compose up --build --force-recreate -d
composer test