Skip to content

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

Closed
morozov wants to merge 1 commit intophp:masterfrom
morozov:bug77118
Closed

Handle invalid index passed to PDOStatement::fetchColumn() as error#3655
morozov wants to merge 1 commit intophp:masterfrom
morozov:bug77118

Conversation

@morozov
Copy link
Copy Markdown
Contributor

@morozov morozov commented Nov 7, 2018

Fixes bug #77118.

@petk petk added the Bug label Nov 7, 2018
@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Nov 7, 2018

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

@morozov
Copy link
Copy Markdown
Contributor Author

morozov commented Nov 7, 2018

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

@morozov
Copy link
Copy Markdown
Contributor Author

morozov commented Nov 7, 2018

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

@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Nov 8, 2018

@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
Copy link
Copy Markdown
Member

nikic commented Nov 8, 2018

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

@cjbj
Copy link
Copy Markdown
Contributor

cjbj commented Nov 8, 2018

I agree with @nikic - an error should occur.

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

@morozov
Copy link
Copy Markdown
Contributor Author

morozov commented Nov 8, 2018

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
Copy link
Copy Markdown
Contributor Author

morozov commented Nov 8, 2018

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

morozov commented Nov 17, 2018

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

@morozov
Copy link
Copy Markdown
Contributor Author

morozov commented Nov 22, 2018

@adambaratz please see the update.

@morozov morozov changed the title Check column number before triggering a warning in PDOStatement::fetchColumn() Handle invalid index passed to PDOStatement::fetchColumn() as error Nov 22, 2018
@php-pulls
Copy link
Copy Markdown

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!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants