Problem/Motivation

This bug was found working on Core integration for Facets module.

When building a Search Page at some point we need to obtain the search pages (entities). In our case from a Deriver class. Here is the code:

      $pages = $this->entityTypeManager->getListBuilder('search_page')->load();

When building the list we initialize the pager at some point. Well, exactly at EntityListBuilder we set the pager and that means the pager will be initialized.

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

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

The problem is that the pager is broken and doesn't appear. I think the solution is to set the $limit to FALSE and avoid to set pagers by default. Manually the pager works as expected with the patch applied.

And here is the related issue: #2626008: Pager doesn't appear on facetsources with facets (core search integration).

Proposed resolution

set $limit property to FALSE by default.

CommentFileSizeAuthor
#8 interdiff-2626402-5-8.txt545 bytesmarthinal
#8 2626402-8.patch3.81 KBmarthinal
#8 2626402-8-only-test-should-fail.patch3.24 KBmarthinal
#5 2626402-5.patch3.79 KBmarthinal
#5 2626402-5-only-test-should-fail.patch3.22 KBmarthinal
#2 2626402-2.patch585 bytesmarthinal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marthinal created an issue. See original summary.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2626402-2.patch585 bytes

Adding the patch.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Issue summary: View changes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 8.1.x-dev » 8.0.x-dev

Hmmmm. I don't know if this is the right thing to do, actually.

It looks like the base EntityListBuilder class sets $limit to 50, which SearchPageListBuilder inherits. So probably the right thing to do is to make sure that when the output of SearchPageListBuilder is shown (or any other EntityListbuilder, for that matter), a pager is included, rather than setting $limit to 0, which would list everything?

Also I don't see why this needs to be 8.1 if it is a bug fix?

marthinal’s picture

marthinal CreditAttribution: marthinal commented
FileSize
2626402-5-only-test-should-fail.patch3.22 KB
2626402-5.patch3.79 KB

@jhodgdon Thanks for reviewing!

Well... I think the best way to explain the problem is creating a new test to reproduce it. :)

function search_pager_test_query_search_node_search_alter(AlterableInterface $query) {
  \Drupal::service('entity_type.manager')->getListBuilder('search_page')->load();
}

Maybe I'm missing something but... I think that we set the $limit(50) by default and that means that the pager is initialized so... the current search page pager is overridden.

The last submitted patch, 5: 2626402-5-only-test-should-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2626402-5.patch, failed testing.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2626402-8-only-test-should-fail.patch3.24 KB
2626402-8.patch3.81 KB
interdiff-2626402-5-8.txt545 bytes

Hmmm locally it works. Let's try again.

The last submitted patch, 8: 2626402-8-only-test-should-fail.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Uh... This is a very weird test.

The problem you are citing is in SearchPageListBuilder. This builds a list of search page entities, which are config entities that represent ways people can search.

The test does a search with paging in the results, and then for some odd reason, in the query alter for search, it loads up a SearchPageListBuilder. Uh, what? ?!? SearchPageListBuilder doesn't have anything to do with search results. The test is just plain odd.

So that makes no sense to me as a test, and the fix is I think still wrong. The problem is that SearchPageListBuilder doesn't properly have its own pager, so most likely in this test it is getting mixed up and trying to override the pager from the search results. So I guess that would happen on any page where you had some pager and also tried to instantiate a SearchPageListBuilder.

And the right fix is to make sure SearchPageListBuilder has its own pager, right?

marthinal’s picture

marthinal CreditAttribution: marthinal commented

@jhodgdon I'm adding the test to reproduce my problem. I don't want to add this test to the core :)

When building a facet we need to alter the query and at some point we call the SearchPageListBuilder to obtain the search page entities.This is because Facets module needs to do it.

So the test is reproducing the problem.

So I guess that would happen on any page where you had some pager and also tried to instantiate a SearchPageListBuilder.

Yes this is problem.

And the right fix is to make sure SearchPageListBuilder has its own pager, right?

For the moment I'm not sure about the correct place to verify it. I need to think a little bit more about it...

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Closed (works as designed)

I've fixed the problem using the storage directly instead of the list_builder. I don't need the list builder here.

$pages = $this->entityTypeManager->getStorage('search_page')->loadMultiple();

Many thanks for you time and help!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

OK! I don't think I helped much, but I'm glad your issue is fixed. ;)