Problem/Motivation

In #3345938: Deprecate support for unused \PDO::FETCH_* modes, we normalized the fetch modes used in Drupal's database operations. However, we fell short of introducing our own logic to indicate the fetch mode, and kept using \PDO::FETCH* constants for the purpose.

This issue is to define fetch modes independently from PDO so that non-PDO db libraries can implement their statement objects without needing to deal with constants that are not in their scope (thinking of the mysqli driver, mostly).

Steps to reproduce

Proposed resolution

  • Introduce an enumeration to define Drupal's supported fetch modes
  • Replace \PDO::FETCH_* constants with the enumeration where used
  • Introduce a trait that introspects calls to PDO methods so that Drupal's statement objects can be abstract from the client library in use (and allow swapping them in contrib drivers)
  • Deprecate usage of \PDO::FETCH_* constants in method calls outside of the above trait

Remaining tasks

User interface changes

Introduced terminology

API changes

The use of the \PDO::FETCH_* constants is deprecated and replaced with the new enum Drupal\Core\Database\Statement\FetchAs values:

  • Drupal\Core\Database\Statement\FetchAs::Associative (\PDO::FETCH_ASSOC)
  • Drupal\Core\Database\Statement\FetchAs::ClassObject (\PDO::FETCH_CLASS | \PDO::FETCH_PROPS_LATE)
  • Drupal\Core\Database\Statement\FetchAs::Column (\PDO::FETCH_COLUMN)
  • Drupal\Core\Database\Statement\FetchAs::List (\PDO::FETCH_NUM)
  • Drupal\Core\Database\Statement\FetchAs::Object (\PDO::FETCH_OBJ)

Data model changes

Release notes snippet

Issue fork drupal-3487851

Command icon Show commands

Start 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:

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Active » Needs work
mondrake’s picture

Assigned: mondrake » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#3345938: Deprecate support for unused \PDO::FETCH_* modes
mondrake’s picture

Issue summary: View changes
daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

For the change record and the questions on the MR.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
mondrake’s picture

Follow up: #3488467: Introduce a StatementBase abstract class, to create a common base class for statement objects.

daffie’s picture

Status: Needs review » Needs work

The CI pipeline is not happy.

mondrake’s picture

Status: Needs work » Needs review

Juggled a bit with typehints.

daffie’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

All the code change good to me.
I have updated the CR and the IS.
The CI pipeline is green for all by core supported databases.
All my questions have been answered.
For me it is RTBC.

mondrake’s picture

Thanks for review @daffie!

mondrake’s picture

I'm still trying to do some tuning with the typehints in PdoTrait - to make them as accurate and PHPStan-proof as possible. It may turn out that CodeSniffer will be unhappy, in which case I think we should silence its failures.

mondrake’s picture

Status: Reviewed & tested by the community » Needs review

Changes to return typehints were OK for CodeSniffer, good - but now this needs review again I suppose, sorry @daffie

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Stricter typing is highlighting a bug. On that.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Reviewable again.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All looks good to me.
Back to RTBC.

mondrake’s picture

Status: Reviewed & tested by the community » Needs review

Found a glitch while working on the follow up.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Change look good to me.
Back to RTBC.

mondrake’s picture

Status: Reviewed & tested by the community » Needs review

Rebased after commit of #3489538: StatementPrefetchIterator::fetchField fails silently if the column index is out of range. I swapped responsability between ::fetchColumn() and ::fetchField() in StatementPrefetchIterator, very minor but I think this requires to be set back to NR. Sorry.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Change look good to me.
Back to RTBC.

  • longwave committed feff7673 on 11.x
    Issue #3487851 by mondrake, daffie: Replace \PDO::FETCH_* constants to...
longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed feff767 and pushed to 11.x. Thanks!

Also published the change record.

Status: Fixed » Closed (fixed)

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

cafuego’s picture

It would be quite nice if the Drupal phpcs rules complained about the use of PDO:: contants in (custom) code.

And/or add support to use PDO::FETCH_GROUP via FetchAs. That one seems to be missing and now Drupal complains about an unsupported fetch mode that PHP supports just fine.

lys_mykyta made their first commit to this issue’s fork.

rob230’s picture

Why wasn't FETCH_GROUP added as an option? Is there a way to replicate that behaviour as it was in D10?

Edit: found issue #3546916: Enhance database fetch mode to support PDO::FETCH_GROUP and combination with bitwise operator for dealing with that.