Problem/Motivation

From #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding.

StatementPrefetch::fetch() has some code that allows emulating different PDO fetch modes, since all data is prefetched on execution in FETCH_ASSOC format, and needs to be re-formatted according to the fetch mode used in each fetch. This code could be reused for contrib drivers that are not based on PDO.

Also, the initialization of a FETCH_CLASS fetch object with constructor arguments is based on reflection - since PHP 5.6 we could have been using the splat operator (...) instead.

Proposed resolution

  • Extract the fetch emulation code in a trait with appropriate protected methods that can be used in statement classes.
  • Implement that in the StatementPrefetch.

Remaining tasks

User interface changes

none

API changes

A new trait with protected methods.

Data model changes

none

Release notes snippet

CommentFileSizeAuthor
#2 3347497-2.patch7.24 KBmondrake

Issue fork drupal-3347497

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

StatusFileSize
new7.24 KB

mondrake’s picture

Status: Postponed » Needs review

Parent got committed. Put the patch in a MR, for review.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
We already have testing for all the changes in the database FetchTest.
For me it is RTBC.

mondrake’s picture

FWIW, just implemented the trait in the mysqli driver on Github, https://github.com/mondrake/mysqli/commit/52da30ef0f1ab47d18ace0c7f4e89d...

All good.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

amber himes matz’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Looks like this MR needs a rebase, as the merge is blocked. It might work to just click the Rebase button in the MR link.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Done

  • catch committed b156dbda on 11.x
    Issue #3347497 by mondrake, daffie: Introduce a FetchModeTrait to allow...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good to me, will make more sense once we add the mysqli driver but chicken-and-egg with that.

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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