Problem/Motivation

KeyValueStoreInterface provides a different set of methods to access to the values stored there.

However, for large collections with a big number of items, the getAll() method can be problematic and cause memory issues.

To mitigate this, an option would be to get a list of item ids in the collection, and use chunks or batch API to process all of them.

To allow this, we would need a getAllKeys() method.

Proposed resolution

Add a new method getAllKeys() to Drupal\Core\KeyValueStore\KeyValueStoreInterface.

Remaining tasks

Add new method and tests.

User interface changes

N/A

Introduced terminology

N/A

API changes

Add a new method getAllKeys() to Drupal\Core\KeyValueStore\KeyValueStoreInterface.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 3563639-nr-bot_ncagsvoq.txt549 bytesneeds-review-queue-bot
#16 3563639-nr-bot_cd0mw00_.txt1.8 KBneeds-review-queue-bot

Issue fork drupal-3563639

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:

About issue forks

Comments

plopesc created an issue. See original summary.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Title: Add getAllKeys() method to KeyValueStorage » Add getAllKeys() method to KeyValueStoreInterface
Issue summary: View changes

plopesc opened merge request !14120

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue summary: View changes
Status: Active » Needs review

Moving to NR.

This new method would be helpful for scenarios like the one described in #3542436: Cleanup key_value table.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Issue tags: +Needs change record

Can we get a change record for it please.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue tags: -Needs change record

Change record draft added. Thank you for the heads-up!

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

CR reads fine to me

Seems like a straight forward addition no comment.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Thanks for the idea. I have two concerns here:

  1. Aren't return types on new methods required now?
  2. Since we are talking scalability, whether in native or in documentation I would love to see array|\Traversable instead of array
nicxvan’s picture

nicxvan commented
Status: Reviewed & tested by the community » Needs work

I'm going to set to needs work for 9.

9.1 is correct.
9.2 I agree.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Thank you for your review!

Added return type hinting. Used iterable instead of array|\Traversable. From what I read, it is equivalent.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Ah yes, thanks, sorry I forgot they fixed the iterable issues in PHP 8.2.0, all good.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

catch
he/him
Primary language English
commented

Adding a method to an interface is allowed per https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

However, I think we should check contrib to see if there are actual implementations around, and probably open issues against those modules if so.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3563639-nr-bot_cd0mw00_.txt1.8 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Bot rebellion

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Moving to needs review for #15.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Needs work

Made some searches in https://search.tresbien.tech/ and found some possible usages where all the items were loaded, but only the keys were used:

https://git.drupalcode.org/project/canvas/-/blob/1.x/src/ComponentIncomp...
https://git.drupalcode.org/project/project_browser/-/blob/2119816e37927d...

Also found a place in core where we could take advantage of this new method:
https://git.drupalcode.org/project/drupal/-/blob/9fc3187d7cc206c9101a22b...

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Back to NR, once this gets merged, we could open issues in contrib modules above to let them know that they could use the new function.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3563639-nr-bot_ncagsvoq.txt549 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

believe the bot got this one wrong.