Found while working on #3488467: Introduce a StatementBase abstract class.
StatementPrefetchIterator::fetchField() fails silently if the column index is out of range.
The corresponding behaviour for StatementPrefetchIterator, that directly invokes PDOStatement::fetchColumn(), throws a \ValueError error in this case.
Add check of existence of the column index and if not, throw \ValueError to be consistent with PDO.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
mondrakeComment #4
mondrakeComment #5
daffie commentedComment #6
mondrake@daffie MySql and PostgresSql are throwing
\ValueErrortoday, so the 3 core drivers behave differently from each other ATM.It seemed to me that aligning SQLite would be the best option given that anyway it's more unlikely to be used in production. Besides, it's really edge case.
Comment #7
alexpottOne thing thats odd here is that
$this->connection->query('SELECT [name] FROM {test} WHERE [age] = :age', [':age' => 255])->fetchField(200)and
$this->connection->query('SELECT [name] FROM {test} WHERE [age] = :age', [':age' => 25])->fetchField(200)result in differences regardless of DB driver. When we return no rows we return FALSE regardless of whether the column exists. Whereas when we return a row on mysql and psql if the column does not exist we throw a ValueError whereas on sqlite we return FALSE.
I'm not sure yet what the correct behaviour here is.
Comment #8
alexpottWe should definitely ensure that there is testing for fieldField() when there are no rows returned.
Comment #9
mondrakeWill look into this, thanks
Comment #10
mondrakeTL;DR
Full story:
fetchField()is historically an alias of\PDOStatement::fetchColumn()- it is so since #225450: Database Layer: The Next Generation.So IMHO we should make a non-PDO implementation behave as closely as possible to its original.
Now,
\PDOStatement::fetchColumn()documentation says (emphasis mine)returning false is btw a potential issue, as the same documentation makes clear:
However, this is mitigated in Drupal by the fact that all values returned from the statement are strings, by setting
\PDO::ATTR_STRINGIFY_FETCHESin the connection options.So, the fetch will return a PHP string if a value is available (independently from its db representation), and a PHP bool === false if no rows are available.
If the required column index is out of range,
\PDOStatement::fetchColumn()throws a\ValueError(it's not documented, but the test here proves that). This makes sense because if we were returning a bool false instead, we would be again unable to distinguish between no more rows and no column defined, which are very different situations - no more rows is a legitimate runtime state, whereas a wrong column index means most likely the query was built wrong. I think in the emulated fetch of the prefetched data we should do the same.Comment #11
mondrakefixed docs and added a test
Comment #12
mondrakeadded one more test case
Comment #13
daffie commentedAll code changes look good to me.
Hoping that the change will not result in a BC break on site in the wild.
I leave it to the committer to decide if we can do this in a minor version or we shall have to wait for a new major version.
For me it is RTBC.
Comment #14
mondrakeInterestingly, trying to insert FALSE in a nullable database field behaves differently between MySql and SQLite - MySql fails hard at insert, SQLite inserts something that is returned as an empty string when reading back.
Working on a test case that can be db abstract.
Comment #15
mondrakeOk, so empty resultset prevails on missing column index in PDO, and prefetch works the same. I think we are covered now.
Comment #16
alexpottI suspect all that we have different behaviour for the \Drupal\Core\Database\StatementInterface::fetchCol() method. Maybe a separate issue.
Comment #17
mondrakeAdded #3490200: StatementPrefetchIterator::fetchCol fails silently if the column index is out of range as a followup for #16, and replied inline referencing existing issues to deprecate StatementPrefetchIterator::fetchField.
Comment #18
daffie commentedAll code changes look good to me.
All points have been addressed.
I leave it to the committer to decide if we can do this in a minor version or we shall have to wait for a new major version.
For me it is RTBC.
Comment #19
alexpottLet's put this in 11.x and leave it to 11.2.0 for release. I think the change in behaviour of sqlite to match mysql and postgres is fine but we're already in 11.1 beta so this does not belong there.