Fix Third parameter not allowed for PDO::FETCH_COLUMN #4173

BenMorel · 2020-07-19T23:23:12Z

Q	A
Type	bug
BC Break	no
Fixed issues	#3975

Summary

I'm getting the following error with PHP 7.4 (tested on 7.4.5 and 7.4.8):

PDOException: SQLSTATE[HY000]: General error: Third parameter not allowed for PDO::FETCH_COLUMN in (...)/vendor/doctrine/dbal/lib/Doctrine/DBAL/Statement.php:256

This only affects PDOStatement, that does not seem to accept null anymore to skip the 3rd parameter.

This fix calls the method with 2 parameters only, when the third argument is null.

greg0ire · 2020-07-21T07:24:08Z

Triggering a new AppVeyor build…

greg0ire

Maybe there should be a test asserting that fetchAll can be used without a third argument?

Also, I could swear I have seen something similar to this PR somewhere, but I can't find it… maybe you remember @morozov ?

morozov · 2020-07-21T16:21:24Z

Maybe there should be a test asserting that fetchAll can be used without a third argument?

I believe this bug is only possible with an externally initialized PDO connection which is a dirty hack deprecated in 2.11.x. The DBAL’s wrapper over the PDO statement already does this. A test is a must.

Also, I could swear I have seen something similar to this PR somewhere, but I can't find it… maybe you remember @morozov ?

I remember seeing something similar too. Let me try to find it.

morozov · 2020-07-21T16:34:28Z

@greg0ire I believe you were talking about #3984.

greg0ire · 2020-07-21T17:10:49Z

@BenMorel tell us if you need help writing the test. Also, please have a look at #3984, it handles one more case.

greg0ire · 2020-07-21T17:12:25Z

cc @duncan3dc

BenMorel · 2020-07-21T20:06:42Z

I'm happy to take over #3984, unless @duncan3dc completes his PR by adding tests!

duncan3dc · 2020-07-21T20:17:27Z

@BenMorel that would be super if you have the time

BenMorel · 2020-07-21T21:58:18Z

Alright, I took over #3984, and added tests. Actually it looks like Statement::fetchAll() was never tested with 2 and 3 arguments, so this is a useful addition!

My tests highlighted an issue, though: Travis shows that mysqli, sqlsrv and ibm_db2 seem to ignore the second parameter for FetchMode::COLUMN, and return the first column's value instead.

Any idea why?

morozov · 2020-07-21T22:21:44Z

Any idea why?

I suspect that DBAL exposes these parameters only because PDO has them and DBAL literally implements the PDO APIs by extending the corresponding classes. At the same time, the underlying drivers don't have a similar API and nobody really cared about using them. FWIW, all these parameters will be removed in 3.0.x.

BenMorel · 2020-07-21T22:52:26Z

Makes sense, but what should we do in the meantime? It's hard to test something that's broken!

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

morozov · 2020-07-21T23:16:32Z

[…] what should we do in the meantime?

You don't have to cover all drivers. It's fine if you exclude the ones that don't implement the API being tested.

BenMorel · 2020-07-24T00:40:44Z

All good for me now, ready for review!

morozov · 2020-07-24T00:49:17Z

The test doesn't seem to reproduce the problem being fixed:

$ git checkout 2.10.x
$ git checkout d23530c08a53eacff089123e87443b7224293893 -- tests/Doctrine/Tests/DBAL/Functional/StatementTest.php tests/Doctrine/Tests/DBAL/Functional/StatementTestModel.php
$ phpunit -c pdo-sqlite.phpunit.xml tests/Doctrine/Tests/DBAL/Functional/StatementTest.php
PHPUnit 8.5.5 by Sebastian Bergmann and contributors.

.......................                                                           23 / 23 (100%)

Time: 74 ms, Memory: 12.00 MB

OK (23 tests, 34 assertions)

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

BenMorel · 2020-07-24T22:30:08Z

The test doesn't seem to reproduce the problem being fixed

I'm not getting the error anymore either. I definitely was. I'm not sure what I'm missing?

morozov · 2020-07-24T22:41:42Z

I'm not sure what I'm missing?

Probably, #4173 (comment).

BenMorel · 2020-07-24T22:54:39Z

Probably, #4173 (comment).

Should I create a separate test class that initializes a connection this way? The problem is, it will run whatever the configured database driver.

morozov · 2020-07-24T23:05:20Z

I genuinely don't understand why you want to fix the behavior multiple aspects of which are deprecated:

Passing an external PDO is deprecated.
The object fetch mode is deprecated.

The very fact that it's broken is a natural boundary that makes people not use it. By relying on it, you'll make it impossible to upgrade to DBAL 3.0 which is the only version that will support PHP 8.

Should I create a separate test class that initializes a connection this way?

You can try whatever approach that makes this test fail w/o the fix. We can discuss the actual implementation once it's submitted.

BenMorel · 2020-07-27T22:24:25Z

Passing an external PDO is deprecated.

I know that now, but my app is heavily using PDO together with ORM/DBAL (for example through my BulkInserter package), and that stuff would need to be refactored to use the DBAL instead, or a separate DB connection.

The object fetch mode is deprecated.

I'm only using the object fetch mode in tests. When I encountered the bug, I was simply using PDO::FETCH_COLUMN, which is not deprecated AFAIK, and breaks as well.

You can try whatever approach that makes this test fail w/o the fix. We can discuss the actual implementation once it's submitted.

I will, as soon as I can.

BenMorel · 2020-09-23T21:42:00Z

@morozov Getting back to this.

I'm now creating a Connection with an externally initialized PDO instance. I'm only doing it for SQLite, using an in-memory connection, that doesn't require any configuration. I'm detecting if we're running on the PDOSqliteDriver, to avoid repeating the tests for other drivers.

I'm not running this test on other PDO drivers, as creating connections with external PDO instances would require duplicating the driver's logic to create the DSN, etc., which I feel is way overkill for what should be a quick fix for an issue that won't exist in 3.0.

The test does now reproduce the problem being fixed:

$ git checkout 2.10.x
$ git checkout 9d08344119df519ddb76a9b0a38bbc9dccf70757 -- tests/Doctrine/Tests/DBAL/Functional/StatementTest.php tests/Doctrine/Tests/DBAL/Functional/StatementTestModel.php
$ vendor/bin/phpunit tests/Doctrine/Tests/DBAL/Functional/StatementTest.php
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.

....................ESS                                           23 / 23 (100%)

Time: 372 ms, Memory: 8.00 MB

There was 1 error:

1) Doctrine\Tests\DBAL\Functional\StatementTest::testFetchAllWithOneArgumentOnExternalPDO
Exception: [PHPUnit\Framework\Error\Warning] PDOStatement::fetchAll(): SQLSTATE[HY000]: General error: Extraneous additional parameters

morozov

You'll have to retarget this to 2.11.x since 2.10.x only accepts security fixes.

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

morozov · 2020-09-23T23:44:17Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+        $driver = $this->connection->getDriver();
+
+        if (! $driver instanceof PDOSqliteDriver) {
+            $this->markTestSkipped('External PDO connection tests run only with an SQLite configuration.');


Why does this test require SQLite?

As I mentioned in #4173 (comment), I think it's overkill to test every PDO driver here, so I've set it up for in-memory SQLite only. And I thought it's more logical to only actually run this test when we're running with an SQLite config, then?

Actually, I've had to remove this check as I'm not extending DbalFunctionalTestCase anymore, so I don't have access to the currently configured driver. May I'd need to re-introduce a condition before running these tests, either reading from the global config, or at least check that PDO SQLite is installed?

I think it's overkill to test every PDO driver here […]

Overkill in what? Run time, development time, maintenance? The test suite is designed to run all functional tests with all supported configurations. You don't have to reimplement that from scratch.

BenMorel · 2020-09-24T07:16:07Z

Rebased & ready for review.

greg0ire · 2020-09-24T07:17:58Z

@BenMorel there is a CS issue

BenMorel · 2020-09-24T07:19:06Z

@greg0ire Fixed!

BenMorel · 2020-09-24T07:25:35Z

@greg0ire Sorry, just pushed a new commit! This dismissed your review.

greg0ire

We should remember to squash merge this, 16 commits is a bit much :P

bytehead

💪 Thanks, works in my case!

morozov · 2020-09-24T16:27:58Z

tests/Doctrine/Tests/DBAL/Functional/ExternalPDOInstanceTest.php

+
+    protected function setUp(): void
+    {
+        $pdo = new PDO('sqlite::memory:');


Actually, I've had to remove this check as I'm not extending DbalFunctionalTestCase anymore, so I don't have access to the currently configured driver.

What will happen here if a run the test suite with say sqlsrv configuration and don't have pdo_sqlite installed? All existing functional tests have proper checks for the installed extensions and current configuration. Why do you need it to not be a functional one?

May I'd need to re-introduce a condition before running these tests, either reading from the global config, or at least check that PDO SQLite is installed?

I'd write a proper functional test that:

Skips if the currently used connection is not PDO.

Extract PDO from the connection.

Create a new connection with external PDO.

Proceed with testing.

Good point, I forgot that PDOConnection extends PDO in 2.x, so I can indeed get the PDO instance from the existing Connection to create a new one. Done!

Actually, while the tests do pass, they also pass on 2.10 now I'm afraid. This is because the PDO we extract from the Connection is a PDOConnection, not a raw PDO. So we're back to square one: there does not seem to be a way to run this test for all PDO drivers, without creating our own PDO instances, which requires duplicating the PDO creation logic from the driver (DSN etc.) in the tests.

This is what I called overkill in development time, for a quick fix that will be gone in 3.0 anyway.

This is because the PDO we extract from the Connection is a PDOConnection, not a raw PDO.

Uh... you're right. This is why we're getting rid of this untestable inheritance. Please restore the previous version and squash.

This is what I called overkill in development time

I can't agree. Fixing something that is not supposed to be used may be overkill. Proper testing is not.

Please restore the previous version and squash.

Done! And re-introduced the PDO SQLite driver check to only run tests on CI that target SQLite, and prevent it from running when SQLite might not be available.

morozov · 2020-09-24T16:30:14Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+        $driver = $this->connection->getDriver();
+
+        if (! $driver instanceof PDOSqliteDriver) {
+            $this->markTestSkipped('External PDO connection tests run only with an SQLite configuration.');


I think it's overkill to test every PDO driver here […]

Overkill in what? Run time, development time, maintenance? The test suite is designed to run all functional tests with all supported configurations. You don't have to reimplement that from scratch.

BenMorel · 2020-09-24T20:26:08Z

@morozov Ready for review!

morozov · 2020-09-25T22:03:18Z

Thanks, @BenMorel.

BenMorel · 2020-09-25T22:09:15Z

Thank you!

@greg0ire

Release [2.11.1](https://github.com/doctrine/dbal/milestone/80) 2.11.1 ====== - Total issues resolved: **2** - Total pull requests resolved: **8** - Total contributors: **6** Documentation ------------- - [4299: Link to contributing guide](doctrine#4299) thanks to @greg0ire SQLite,Test Suite,pdo_sqlite ---------------------------- - [4297: Fix ExceptionTest::testConnectionExceptionSqLite() on macOS](doctrine#4297) thanks to @morozov - [4296: Increase indent in definition lists](doctrine#4296) thanks to @greg0ire Deprecation,Prepared Statements ------------------------------- - [4291: Deprecate Abstraction\Result](doctrine#4291) thanks to @morozov BC Fix,Quoting -------------- - [4287: Restore PDOStatement::quote() for backward compatibility](doctrine#4287) thanks to @morozov and @Shahelm BC Fix,Query ------------ - [4286: Fix BC break: QueryBuilder::andWhere() etc. should ignore empty strings](doctrine#4286) thanks to @BenMorel and @infabo Bug,Documentation,Prepared Statements ------------------------------------- - [4285: Fix phpdoc on deprecated functions](doctrine#4285) thanks to @qdequippe Bug,PDO,Prepared Statements --------------------------- - [4173: Fix Third parameter not allowed for PDO::FETCH&doctrine#95;COLUMN](doctrine#4173) thanks to @BenMorel # gpg: Signature made Sun Sep 27 06:35:40 2020 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key

greg0ire added the Bug label Jul 21, 2020

greg0ire closed this Jul 21, 2020

greg0ire reopened this Jul 21, 2020

greg0ire reviewed Jul 21, 2020

View reviewed changes

greg0ire closed this Jul 21, 2020

greg0ire reopened this Jul 21, 2020

morozov added PDO Prepared Statements labels Jul 21, 2020

greg0ire linked an issue Jul 21, 2020 that may be closed by this pull request

v2.10.2 - SQLSTATE[HY000]: General error: Extraneous additional parameters #3975

Closed

morozov reviewed Jul 21, 2020

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php Outdated Show resolved Hide resolved

morozov reviewed Jul 24, 2020

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php Outdated Show resolved Hide resolved

morozov reviewed Jul 24, 2020

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php Outdated Show resolved Hide resolved

duncan3dc mentioned this pull request Sep 2, 2020

Only pass the additional arguments if they are not null #3984

Closed

morozov reviewed Sep 23, 2020

View reviewed changes

BenMorel changed the base branch from 2.10.x to 2.11.x September 24, 2020 06:52

greg0ire previously approved these changes Sep 24, 2020

View reviewed changes

BenMorel dismissed greg0ire’s stale review via 4427047 September 24, 2020 07:23

greg0ire previously approved these changes Sep 24, 2020

View reviewed changes

bytehead previously approved these changes Sep 24, 2020

View reviewed changes

greg0ire requested a review from morozov September 24, 2020 11:15

morozov reviewed Sep 24, 2020

View reviewed changes

BenMorel dismissed stale reviews from bytehead and greg0ire via 56b2010 September 24, 2020 19:36

Fix 2nd/3rd parameter not allowed for PDO::FETCH_COLUMN

35c778b

BenMorel force-pushed the 2.10.x-fix-third-parameter branch from 1a8989f to 35c778b Compare September 25, 2020 21:39

morozov approved these changes Sep 25, 2020

View reviewed changes

morozov added this to the 2.11.1 milestone Sep 25, 2020

morozov merged commit 787c141 into doctrine:2.11.x Sep 25, 2020

github-actions bot mentioned this pull request Sep 27, 2020

Merge release 2.11.1 into 3.0.x #4303

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022

Fix Third parameter not allowed for PDO::FETCH_COLUMN #4173

Fix Third parameter not allowed for PDO::FETCH_COLUMN #4173

Conversation

BenMorel commented Jul 19, 2020 • edited by greg0ire Loading

Summary

greg0ire commented Jul 21, 2020

greg0ire left a comment

Choose a reason for hiding this comment

morozov commented Jul 21, 2020

morozov commented Jul 21, 2020

greg0ire commented Jul 21, 2020

greg0ire commented Jul 21, 2020

BenMorel commented Jul 21, 2020

duncan3dc commented Jul 21, 2020

BenMorel commented Jul 21, 2020

morozov commented Jul 21, 2020

BenMorel commented Jul 21, 2020

morozov commented Jul 21, 2020

BenMorel commented Jul 24, 2020

morozov commented Jul 24, 2020

BenMorel commented Jul 24, 2020

morozov commented Jul 24, 2020

BenMorel commented Jul 24, 2020

morozov commented Jul 24, 2020 • edited Loading

BenMorel commented Jul 27, 2020

BenMorel commented Sep 23, 2020

morozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorel commented Sep 24, 2020

greg0ire commented Sep 24, 2020

BenMorel commented Sep 24, 2020

BenMorel commented Sep 24, 2020 • edited Loading

greg0ire left a comment

Choose a reason for hiding this comment

bytehead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorel commented Sep 24, 2020

morozov commented Sep 25, 2020

BenMorel commented Sep 25, 2020

BenMorel commented Jul 19, 2020 •

edited by greg0ire

Loading

morozov commented Jul 24, 2020 •

edited

Loading

BenMorel commented Sep 24, 2020 •

edited

Loading