Dropped handling of one-based numeric arrays of parameters in Statement::execute()

[morozov]

Q	A
Type	improvement
BC Break	yes

The logic of handling one-based arrays existed in OCI8Statement (57a97eb) and SQLSrvStatement (#110) since their initial implementation without any explanation of why it was needed. Apart from that, the APIs with semantics implicitly defined by input are often more error-prone.

This behavior is not portable. Consider the following test:

/**
 * @dataProvider paramsProvider
 */
public function testHasZeroKey(array $params) : void
{
    $sql  = $this->connection->getDatabasePlatform()->getDummySelectSQL('?, ?');
    $stmt = $this->connection->prepare($sql);
    $stmt->execute($params);

    self::assertEquals([1, 2], $stmt->fetch(FetchMode::NUMERIC));
}

public static function paramsProvider() : iterable
{
    return [
        'zero-based' => [
            [0 => 1, 1 => 2],
        ],
        'one-based' => [
            [1 => 1, 2 => 2],
        ],
        'arbitrary-keys' =>
        [
            [28 => 1, 71 => 2],
        ],
    ];
}

Currently, it produces the following results:

RUNNING TESTS WITH CONFIG ibm-db2.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

EEE (for some reason, the test doesn't work here)                   3 / 3 (100%)

RUNNING TESTS WITH CONFIG mysqli.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

...                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG oci8.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

..E                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG pdo-mysql.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

.EE                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG pdo-oci.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

.EE                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG pdo-pgsql.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

.EE                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG pdo-sqlite.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

.EE                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG pdo-sqlsrv.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

.EE                                                                 3 / 3 (100%)

RUNNING TESTS WITH CONFIG sqlsrv.phpunit.xml
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

..E                                                                 3 / 3 (100%)

As you can see, the 2nd test passes only on sqlsrv, oci8 (where the logic being removed is explicitly implemented) and mysqli (which doesn't care about the keys at all because it binds parameters using a foreach loop).

Additionally, the test that covers the logic implemented in OCI8Statement currently fails on PHP 8 (#3802) and, if kept, would have to be reworked for #3808.

In the past, the test was the source of extra maintenance for no real reason: #3642, #3369, #2854, #3507, #3552.

[beberlei]

@morozov I believe the reason this exists is ->bindParam(1, ...)->bindParam(2, ...) in QueryBuilder.

[morozov]

@beberlei yes, this logic might have been introduced in some drivers because it was needed at the time of introduction but it's not currently needed. Statement::bindParam() and Statement::execute($params) are different API from the DBAL perspective and the perspective of the most of the drivers (some may implement one via the other but it's an implementation detail). The functional tests like DataAccessTest::testPrepareWithExecuteParams() and ::testPrepareWith*() that still pass prove that.

Additionally, as the test results above show, this behavior is not portable and cannot be relied upon. It works as the test expects only in the drivers that artificially implement this behavior and mysqli (where the keys are simply ignored).

[beberlei]

I have to honestly say that I am not to deep to give a good assessment, but the fact that this is not portable across different connections is reason enough to get rid of it. My problem is that this is extremely "hidden" for users in a way that it could lead to sutble breaks if someone would rely on it.

[morozov]

My problem is that this is extremely "hidden" for users in a way that it could lead to subtle breaks if someone would rely on it.

This is another reason to get rid of this hidden gem in a major release. The change is documented in the upgrade notes and can be elaborated on in greater detail later.

I have to honestly say that I am not too deep to give a good assessment […]

The fact that no one is able to give a good assessment should not be a reason to not proceed with the changes. Otherwise, we'll get stuck maintaining the behavior that no one owns and no one understands forever. This is very unproductive and demotivating.

If we merge it like this, as soon as someone comes up with a reason why this change should not have been made, we can reassess it and either revert or provide an alternative solution.

[morozov]

@beberlei could you reapprove, please? I had to resolve a conflict in the upgrade notes with the changes from #3807.

[SenseException]

These changes also seem to be in sync with the QueryBuilder documentation, where 0 is used for the first parameter. It doesn't though with the Data Retrieval chapter. To prevent the communication of mixed usages, I suggest to adapt the parameter indexes to start with 0: https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/data-retrieval-and-manipulation.html#dynamic-parameters-and-prepared-statements

OCI8/Oracle only supports named parameters, but Doctrine implements a client side parser to allow positional parameters also.

Could this be somehow related to the zero-index decision?

[morozov]

To prevent the communication of mixed usages, I suggest to adapt the parameter indexes to start with 0

@SenseException, which exactly method do you mean? As pointed out int #3809 (comment),

Statement::bindParam() and Statement::execute($params) are different APIs

The former will accept 1-based keys (this did and does work for all DBAL drivers and is true for all underlying drivers that support binding a single parameter natively (PDO, ibm_db2)), the latter will accept the 0-based ones.

The chapter you're referring to doesn't even mention passing parameters to Statement::execute().

[morozov]

@SenseException if you request changes on a PR, please make sure you can participate in the discussion. This PR is a dependency for #3808 and #3802, so by not replying, you're blocking progress on three issues, not just one.