Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlite non-sequential keys in array throws error when passed to execute with PDO #16081

Open
truecastdesign opened this issue Sep 26, 2024 · 5 comments

Comments

@truecastdesign
Copy link

truecastdesign commented Sep 26, 2024

Description

PHP 8.2

$dsn = 'sqlite:'.$config->database;

$this->obj = new PDO($dsn);
$this->obj->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

The following code:

<?php
// Assuming $pdo is your PDO database connection object
$values
array(12) {
  [0]=>
  string(23) "/powersports-batteries/"
  [1]=>
  string(32) "/powersports-batteries/-catelist"
  [2]=>
  string(31) "/powersports-batteries/-sideNav"
  [3]=>
  string(10) "/testing1/"
  [4]=>
  string(19) "/testing1/-catelist"
  [5]=>
  string(18) "/testing1/-sideNav"
  [6]=>
  string(19) "/testing1/testing4/"
  [7]=>
  string(28) "/testing1/testing4/-catelist"
  [8]=>
  string(27) "/testing1/testing4/-sideNav"
  [12]=>
  string(24) "/testing1/testing5-copy/"
  [13]=>
  string(33) "/testing1/testing5-copy/-catelist"
  [14]=>
  string(32) "/testing1/testing5-copy/-sideNav"
}

// Notice that array keys are non-sequential after 8. I had removed some items from the array and then tried to use that to delete the rows with those titles. Kept getting that error. 

$sql = "DELETE FROM `data` WHERE title IN (" . str_repeat('?,', count($values) - 1) . '?)';
$stmt = $pdo->prepare($sql);

if ($stmt->execute($values)) {
    echo "Rows deleted: " . $stmt->rowCount();
} else {
    $errorInfo = $stmt->errorInfo();
    echo "Error: " . $errorInfo[2];
}

Resulted in this output:

SQLSTATE[HY000]: General error: 25 column index out of range
Error: column index out of range

But I expected this output instead:

Rows deleted: [expected number of rows, e.g., 12]

The $stmt->execute($values) method should use a foreach instead of a for loop or run a:
$values = array_values($values); over the incoming array so people don't have to bang their head against a way trying to figure out why their query will not run.

PHP Version

PHP 8.2

Operating System

CloudLinux v9.4.0

@damianwadley
Copy link
Member

While counterintuitive at first glance, this seems like the correct behavior to me: executing with an array has PHP do the parameter binding, as in bindParam, for you based on the array key. So it binds parameters 0-8 as expected, then parameters 12, 13, and 14 - which aren't valid because there's only twelve parameters (with positions 0-11).
Changing this behavior to be like "if the key is numeric then ignore it and use counter++ instead" breaks the case where an array might be [1=>"second", 0=>"first"].

In other words, the array passed to execute is always treated as an associative array - meaning the key matters. And that string keys correspond to named parameters while numeric keys correspond to positional parameters.

How would you feel if this was more clearly documented? I see there is a user note about this from a while back, but I did have to scroll a ways before I found it:
https://www.php.net/manual/en/pdostatement.execute.php#80499

@truecastdesign
Copy link
Author

In reading the notes, everyone is saying that you can't use an associative array, it has to have numeric indexes so there should be no reason to put them out of order from the question mark placeholders.

Comments from the notes:
// This is wrong!
// $param = array("name" => "apple", "colour" => "red", "colories" => 150);

// Array must be keyed with integers starting from zero
$param = array("apple", "red", 150);
$sth->execute($param);

This is right from the notes

$placeholders = implode(',', array_fill(0, count($key), '?'));

I would simply expect and want execute to take the array items in the order they are in the array without regard to the numerical indexes mattering. What use case is there for [1=>"second", 0=>"first"]; That seems like very poor programming.

I my case I was running a

$array = array_unique($array);

to remove any duplicates. Having to then run this:

$array = array_values($array);

seems like a lot of fuss to make it work right.

It supports
$sth->execute(array(':calories' => $calories, ':colour' => $colour));
and
$sth->execute(array($calories, $colour));
so it must have some way of deciding if it expects and associative array or an indexed array. I assume it checks for question marks in the prepared query to know what kind of array it is.

If PDO is expecting an numeric indexed array, could it not run the array though a function like this?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct {
int key; // Key of the entry
char* value; // Value of the entry
} MapEntry;

char** array_values(MapEntry* map, int size, int* outSize) {
char** values = malloc(sizeof(char*) * size); // Allocate memory for the output array
for (int i = 0; i < size; i++) {
values[i] = strdup(map[i].value); // Duplicate the string to ensure it is safely managed
}
*outSize = size; // Set the output size
return values; // Return the array of values
}

$array = array_values($array);

@cmb69
Copy link
Member

cmb69 commented Sep 27, 2024

Changing the behavior of how the parameters are bound would be a BC break. Certainly doable, but is it worth it?

@cmb69
Copy link
Member

cmb69 commented Sep 28, 2024

Maybe @SakiTakamachi has an opinion about this issue? Otherwise I think this needs the RFC process, or at least proper discussion on the internals mailing list.

@SakiTakamachi
Copy link
Member

IMHO, I think the safest option is to leave it as is and update the documentation.

BC breaks in the cases Christoph describes, and I believe that treating arrays as if they were doing array_values ​​could lead to ambiguities that could result in unintended bugs.

If you strongly prefer this change, you can of course discuss it on the internal mailing list :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants