StatementPrefetchIterator::fetchField fails silently if the column index is out of range

@daffie MySql and PostgresSql are throwing \ValueError today, 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.

One 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.

We should definitely ensure that there is testing for fieldField() when there are no rows returned.

Will look into this, thanks

TL;DR

• returning a string -> valid value
• returning FALSE -> no more rows in resultset
• throw exception -> invalid column index

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)

Returns a single column from the next row of a result set or false if there are no more rows.

returning false is btw a potential issue, as the same documentation makes clear:

... fetchColumn() should not be used to retrieve boolean columns, as it is impossible to distinguish a value of false from there being no more rows to retrieve

However, this is mitigated in Drupal by the fact that all values returned from the statement are strings, by setting \PDO::ATTR_STRINGIFY_FETCHES in 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.

fixed docs and added a test

added one more test case

All 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.

Interestingly, 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.

Ok, so empty resultset prevails on missing column index in PDO, and prefetch works the same. I think we are covered now.

I suspect all that we have different behaviour for the \Drupal\Core\Database\StatementInterface::fetchCol() method. Maybe a separate issue.

Added 📌 Check StatementPrefetchIterator::fetchCol after strenghtening of ::fetchField Active as a followup for #16, and replied inline referencing existing issues to deprecate StatementPrefetchIterator::fetchField.

All 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.

Let'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.

mondrake → closed merge request !10322

Automatically closed - issue fixed for 2 weeks with no activity.