For some reason, we ended up implementing in the database layer a function DatabaseStatementInterface::fetchField() that is nothing more then an alias of PDOStatement::fetchColumn().

Probably as a consequence of that, we forgot to implement DatabaseStatementPrefetch::fetchColumn() in our user-space implementation of PDOStatement. Bummer, because we recently started using it in core (in DateTimeFunctionalTest::testDateFormatStorage()). Let's fix this in DatabaseStatementPrefetch before considering removing this function altogether in Drupal 8.

Files: 
CommentFileSizeAuthor
#5 1266694-remove-fetch-field.patch212.13 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 32,952 pass(es). View
#1 1266694-missing-fetchfield.patch1.16 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 32,907 pass(es). View

Comments

Damien Tournoud’s picture

Status: Active » Needs review
Issue tags: +needs backport to D7
FileSize
1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 32,907 pass(es). View

Here is the quick-fix patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Seems to look fine for me. This is definitive no api change for d7

chx’s picture

Yeah... quite some oversight for SQLite.

Dries’s picture

Title: DatabaseStatementPrefetch doesn't implement fetchColumn() » DatabaseStatementPrefetch has redudant fetchColumn() and fetchField()
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: -Quick fix, -needs backport to D7

Committed to 7.x and 8.x. I'm marking it back as 'needs work' so we can figure out if we want to remove it in 8.x.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
212.13 KB
PASSED: [[SimpleTest]]: [MySQL] 32,952 pass(es). View

I think we do. Patch attached (it's a big patch).

Damien Tournoud’s picture

Title: DatabaseStatementPrefetch has redudant fetchColumn() and fetchField() » DatabaseStatementInterface has redudant fetchColumn() and fetchField()
Crell’s picture

Status: Needs review » Needs work

fetchField() is there very deliberately, for DX. fetchColumn() is a badly named method. We have fetchCol(), too, which does something entirely different. Which is which? I wouldn't be able to keep them apart myself, frankly.

fetchField() was added specifically to avoid that confusion. We wanted the "fetch a single field out of every record, that is, the entire column of data" operation. So that got called fetchCol(), we added fetchField() to retrieve an individual field out of a record (basically an alias of fetchColumn()), and although PDO has fetchColumn() nothing in Drupal uses it, quite deliberately.

(Of course, column vs. field is a long-standing question. Sigh.)

So simply removing fetchField() would be a step backward for DX, as well as a non-small API break. I wouldn't support that unless we thought through the other related methods.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed eeac53e on 8.3.x
    - Patch #1266694 by Damien Tournoud: Fixed DatabaseStatementPrefetch...

  • Dries committed eeac53e on 8.3.x
    - Patch #1266694 by Damien Tournoud: Fixed DatabaseStatementPrefetch...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.