Inconsistent parameter indexes passed to the logging middleware

[mvorisek]

Bug Report

Q	A
Version	3.3.7

Summary

Correct me if this is intended and documented somewhere, otherwise it seems like a bug to me.

Current behaviour

The issue is bind params are strictly 1-based [1] [2], ie. always starting with index 1, not 0.

But in some places like

dbal/src/Schema/SqliteSchemaManager.php

Line 207 in 2aec3f8

$indexArray = $this->_conn->fetchAllAssociative('SELECT * FROM PRAGMA_TABLE_INFO (?)', [$tableName]);

the positional params are passed to DBAL in 0-based fashion.

Quick demo https://3v4l.org/s8hef shows the conversion must happen somewhere in the DBAL, as 0-based params are not accepted by SQLite natively.

I noticed this issue when using Doctrine\DBAL\Logging\SQLLogger::startQuery() which receives the passed params to DBAL, ie. params starting with index 0.

How to reproduce

Can some DBAL connection query method like:

$conn->fetchAllAssociative('SELECT * FROM PRAGMA_TABLE_INFO (?)', [$tableName]);

and notice what params are passed to Doctrine\DBAL\Logging\SQLLogger::startQuery(). Currently, the params start with 0 which is wrong as such params are not valid.

Expected behaviour

I expect Doctrine\DBAL\Logging\SQLLogger::startQuery() to always return params that are valid and can be used to reproduce the logged query /w and /wo DBAL.

I propose DBAL to never accept 0-based params so no transformation is needed. This seems to be already the case with mysqli driver.

[1] https://www.php.net/manual/en/pdostatement.bindparam.php

For a prepared statement using question mark placeholders, this will be the 1-indexed position of the parameter.

[2] https://www.php.net/manual/en/sqlite3stmt.bindparam.php

Positional parameters start with 1.

[morozov]

In my understanding, the positions of parameters bound via Statement::bindParam() are 1-based as in PDO but the array keys of the parameters passed to Statement::execute() are 0-based (I believe this is also the case for PDO given that all PDO-based DBAL drivers work this way).

As far as I can tell, there is no clear standard on the keys and the order of elements of $params passed to the equivalent of Statement::execute() of the underlying drivers. See #5467 (comment) for example.

The only documentation I could find is about the query builder (#4421, #4422):

dbal/docs/en/reference/query-builder.rst

Lines 47 to 48 in 84328cd

	The numerical parameters in the QueryBuilder API start with the needle
	``0``, not with ``1`` as in the PDO API.

Which on its own is odd: in the same codebase one would have to use 1-based positions in Statement::bindParam() and 0-based ones in QueryBuilder::setParameter().

1-based parameter keys when passed to Statement::execute() are documented as invalid in the upgrade notes (#4411):

dbal/UPGRADE.md

Lines 10 to 14 in 9babd9e

	// This is valid (implicit zero-based parameter indexes)
	$conn->fetchNumeric('SELECT ?, ?', [1, 2]);
	// This is invalid (one-based parameter indexes)
	$conn->fetchNumeric('SELECT ?, ?', [1 => 1, 2 => 2]);

[morozov]

I propose DBAL to never accept 0-based params so no transformation is needed. This seems to be already the case with mysqli driver.

If you're talking about the parameters passed as an array to Statement::execute(), it sounds like a bad idea to me. Such parameters are always passed as a hard-coded list. Requiring the caller to add 1-based keys to this list will make such code more complex and error-prone.

[mvorisek]

I extended the demo https://3v4l.org/Zm6rN - PDO accepts only 1-based positional params, 0-based throws immediately. SQLite3 allows 0-based, but the result is wrong. So it seems the "standard" for the native drivers for positional params is 1-based only.

I do not argue 0-based is much more natural in php world and I do even like it more, as only 0-based array can meet the definition php list.

In atk4/data https://github.com/atk4/data/blob/f4ebf2b94f36f0657faf9b1c58ca17e6c604767c/src/Persistence/Sql/Expression.php#L627 we use Statement::bindValue() and I tested it now /w 0-based params - it fails correctly /w pdo_sqlite and mysqli.

So the only issue here is:

dbal/src/Schema/SqliteSchemaManager.php

Line 207 in 2aec3f8

$indexArray = $this->_conn->fetchAllAssociative('SELECT * FROM PRAGMA_TABLE_INFO (?)', [$tableName]);

(and others)

pass the params into SQLLogger::startQuery as 0-based, instead of 1-based. (or pass 0-based, but consistently for all queries)

[morozov]

Personally, I find the execute($params) API quite unfortunate:

1. It expects $params to have numeric keys or string keys depending on what parameters were bound to the statement at runtime, so it's impossible to enforce the expected keys statically.
2. There's no way to prevent mixing positional and named parameters at the API level.
3. The values of $params elements should correspond to the statement parameter types but it's impossible to enforce that statically (see SQLLogger does not log actual params used in query #4602 (comment)).
4. There's no way to make sure statically that all parameters are passed.
5. It doesn't support specifying parameter types and defaults to "string".
6. It doesn't support output parameters.
7. The behavior of passing $params to execute() of a statement that already has bound parameters is undefined. Do parameters get merged? Or do they replace the previously bound parameters? What's the point of being able to use both ways of binding parameters on a single statement?
8. The shortcut methods implemented at the wrapper level accept parameters and their types as two independent arrays of awful types which are impossible to analyze statically:

   dbal/src/Connection.php

   Lines 824 to 825 in 8532d49

   	* @param list<mixed>|array<string, mixed> $params Query parameters
   	* @param array<int, int|string|Type|null>|array<string, int|string|Type|null> $types Parameter types

If we could get rid of it on the driver level, we could hugely improve the API for binding parameters from the type safety standpoint but the remaining API would require writing more code (which isn't a huge concern). But then the user-facing wrapper-level methods like fetchAllAssociative($sql, $params) would still have to remain purely typed or instead of array $params they would have to accept a "preparer" like:

function (Statement $stmt) use ($foo, $bar): void {
    $stmt->bindParam(1, $foo);
    $stmt->bindParam(2, $bar);
}

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.