Handle invalid index passed to PDOStatement::fetchColumn() as error

[morozov]

Fixes bug #77118.

[KalleZ]

cc @cjbj, shouldn't PDO itself check this before dispatching to the column fetcher tho?

[morozov]

To the @KalleZ's point, the same issue may exist in pdo_sqlsrv as well. See DataAccessTest::testFetchColumnNonExistingIndex() from doctrine/dbal.

[morozov]

@KalleZ did you have something like master...morozov:bug77118-alt in mind?

[KalleZ]

@morozov yeah that was something like it I was thinking, that way we don't need to update all drivers (and database vendors don't need to do it either).

But it needs some more input from people who are more familiar with PDO internals than I am, I'm afraid.

[nikic]

Why should trying to fetch an invalid column be silently ignored?

[cjbj]

I agree with @nikic - an error should occur.

Also, shouldn't the PR's test be generic, since the fix is generic?

[morozov]

Why should trying to fetch an invalid column be silently ignored?

For consistency of the behavior with other PDO drivers (MySQL, SQLite, PostgreSQL). However, the documentation doesn't specify any behavior in this case.

[morozov]

I agree with @nikic - an error should occur.

Also, shouldn't the PR's test be generic, since the fix is generic?

Is it acceptable to trigger/throw an error in PHP 7? The test is not generic because the original fix was not generic.

[adambaratz]

The documented behavior for all of the PDO fetch methods is to return false on a failure. PDO can trigger warnings or exceptions, but that behavior is user-specified.

There aren't a lot of documented (or de facto) patterns for when errors should be triggered. It certainly feels right that this is an error state. But this should only be committed to master since that's a new behavior. Applications might not be expecting this, even if it feels like a minor nuance.

What I would expect here:

• Using ZVAL_FALSE instead of ZVAL_NULL
• Calling pdo_raise_impl_error, probably with the HY000 code, to trigger that warning or exception

I can run the test suite with pdo_dblib after this is updated. Agree the added test should be part of the common suite.

[morozov]

@adambaratz thank you for the suggestion. I'll try to implement as soon as I have some spare time.

[morozov]

@adambaratz please see the update.

[php-pulls]

Comment on behalf of adambaratz at php.net:

Merged into master, branches for 7.2 and 7.3. I saw a similar change make it in as a bug fix so did the same here. Thanks for cleaning this up!