Allow EntityListBuilder::getEntityIds() to easily extend the query [#3332872]

Problem/Motivation

I have a case where I want to add a condition to the query in an EntityListBuilder. To do this I have to copy and override the entire getEntityIds() method:

  /**
   * Loads entity IDs using a pager sorted by the entity id.
   *
   * @return array
   *   An array of entity IDs.
   */
  protected function getEntityIds() {
    $query = $this->getStorage()->getQuery()
      ->accessCheck(TRUE)
      ->sort($this->entityType->getKey('id'));

    // Only add the pager if a limit is specified.
    if ($this->limit) {
      $query->pager($this->limit);
    }
    return $query->execute();
  }

By extracting the query to its own method, this would be easier to extend.

Steps to reproduce

Proposed resolution

Add a getQuery() method:

  protected function getEntityIds() {
    return $this->getQuery()->execute();
  }

  protected function getQuery() {
    $query = $this->getStorage()->getQuery()
      ->accessCheck(TRUE)
      ->sort($this->entityType->getKey('id'));

    // Only add the pager if a limit is specified.
    if ($this->limit) {
      $query->pager($this->limit);
    }
    return $query;
  }

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#22	interdiff_21-22.txt	506 bytes	akram khan
#22	3332872-22.patch	1.34 KB	akram khan
#21	interdiff_18-21.txt	480 bytes	akram khan
#21	3332872-21.patch	1.34 KB	akram khan
#18	interdiff_15-18.txt	585 bytes	akram khan
#18	3332872-18.patch	1.32 KB	akram khan
#15	interdiff_8-15.txt	1.13 KB	akram khan
#15	3332872-15.patch	1.29 KB	akram khan
#12	3332872-2.patch	1.02 KB	prabuela
#11	3332872-2.patch.txt	1.02 KB	prabuela
#8	3332872-1.patch	1018 bytes	prabuela
#5	3332872.patch	773 bytes	prabuela

Comments

Comment #1

12 January 2023 at 09:17

longwave created an issue. See original summary.

Comment #2

prabuela commented 12 January 2023 at 09:18

Assigned:

Unassigned

» prabuela

Comment #3

prabuela commented 12 January 2023 at 09:24

Assigned:

prabuela

» Unassigned

Comment #4

prabuela commented 12 January 2023 at 12:12

Assigned:

Unassigned

» prabuela

Comment #5

prabuela commented 12 January 2023 at 12:25

Status	File	Size
new	3332872.patch	773 bytes

Comment #6

prabuela commented 12 January 2023 at 12:28

Assigned:	prabuela	» Unassigned
Status:	Active	» Needs review

Comment #7

longwave

he/him

English

commented 13 January 2023 at 09:24

Status:

Needs review

» Needs work

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -94,6 +94,10 @@ public function load() {
+  protected function getQuery() {

Thanks, this method needs some documentation but this looks good otherwise.

Comment #8

prabuela commented 13 January 2023 at 13:54

Assigned:

Unassigned

» prabuela

Status	File	Size
new	3332872-1.patch	1018 bytes

Comment #9

prabuela commented 13 January 2023 at 13:55

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

Comment #10

joachim commented 13 January 2023 at 14:11

Status:

Needs review

» Needs work

Good idea, just a few things to fix:

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -91,9 +91,19 @@ public function load() {
+   *   Execute the array of entity IDs.

That doesn't make sense as a @return. Does this need changing?

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -91,9 +91,19 @@ public function load() {
+   * Loads entity IDs using the entity id.
+   *
+   * @return array
+   *   An array of entity IDs.

That's wrong.

Looks like the docblocks have got swapped around maybe?

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -91,9 +91,19 @@ public function load() {
+  protected function getQuery() {

As this is a new method, and protected, it should have a return type.

Comment #11

prabuela commented 10 April 2023 at 11:03

Status	File	Size
new	3332872-2.patch.txt	1.02 KB

Comment #12

prabuela commented 10 April 2023 at 11:07

Status	File	Size
new	3332872-2.patch	1.02 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	3332872-2.patch.txt	1.02 KB

Comment #13

prabuela commented 10 April 2023 at 11:07

Status:

Needs work

» Needs review

Comment #14

smustgrave commented 10 April 2023 at 14:58

Status:	Needs review	» Needs work
Issue tags:		+Needs Review Queue Initiative

Seems the points from #10 have not been addressed

Also please include an interdiff with patches

Patch file names also seem incorrect.

Comment #15

akram khan

English

Cuttack, Odisha

commented 10 April 2023 at 18:50

Status	File	Size
new	3332872-15.patch	1.29 KB
new	interdiff_8-15.txt	1.13 KB

added updated patch and address comment #10

Comment #16

prabuela commented 11 April 2023 at 09:43

Status:

Needs work

» Needs review

Comment #17

smustgrave commented 11 April 2023 at 23:56

Status:

Needs review

» Needs work

Still doesn't seem correct

+ protected function getQuery(): QueryInterface {

But the doc is saying an array is returned.

Comment #18

akram khan

English

Cuttack, Odisha

commented 12 April 2023 at 07:29

Status	File	Size
new	3332872-18.patch	1.32 KB
new	interdiff_15-18.txt	585 bytes

made changes according to comment #17

Comment #19

akram khan

English

Cuttack, Odisha

commented 12 April 2023 at 08:33

Status:

Needs work

» Needs review

Comment #20

smustgrave commented 12 April 2023 at 16:38

Status:

Needs review

» Needs work

Still doesn't seem correct.

Someone should check the code vs doing what's just written here.

But this reading now as

getQuery returns query object used to load entity IDs.

But description says Loads entity IDs using the entity id.

Comment #21

akram khan

English

Cuttack, Odisha

commented 12 April 2023 at 18:42

Status:

Needs work

» Needs review

Status	File	Size
new	3332872-21.patch	1.34 KB
new	interdiff_18-21.txt	480 bytes

made changes now updated comment is more accurate and clarifies that the getQuery() method returns a query object for loading entity IDs from the storage

Comment #22

akram khan

English

Cuttack, Odisha

commented 12 April 2023 at 19:02

Status	File	Size
new	3332872-22.patch	1.34 KB
new	interdiff_21-22.txt	506 bytes

fixed CCF #21

Comment #23

smustgrave commented 12 April 2023 at 20:44

Status:

Needs review

» Reviewed & tested by the community

Think it's ready for committer review.

Comment #24

30 April 2023 at 18:26

longwave committed 46853e85 on 10.1.x

Issue #3332872 by Akram Khan, PrabuEla, smustgrave, longwave, joachim:...

Comment #25

longwave

he/him

English

commented 30 April 2023 at 18:26

Status:

Reviewed & tested by the community

» Fixed

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -91,9 +92,19 @@ public function load() {
-   *   An array of entity IDs.
+   *   An array of entities.

It's still an array of entity IDs, the return type here hasn't changed. I fixed this on commit.

Committed and pushed 46853e85a1 to 10.1.x. Thanks!

Comment #26

14 May 2023 at 18:29

Status:

Fixed

» Closed (fixed)

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

Allow EntityListBuilder::getEntityIds() to easily extend the query

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Referenced by