The facet links contain a query parameter for "page" which can lead to false dead-ends in search results views.
Example:
* View with 10 items per page.
* Click on page 6.
* Facet links now have a "&page=5" (indexed at 0).
* Click on a facet that has a count < 50 (5 pages x 10 items per page). Example: TermX with only 2 items.
* Current result: Views empty text would be shown (if configured) since there is no page 6 for the search results with the TermX facet active.

Expected result:
* The facet link does not have a "&page=5".
* Click on the facet link takes you to the 1st page for the results.

CommentFileSizeAuthor
#13 facet_links_should_be-2898189-13.patch2.34 KBrecrit
#9 facets-debugging-2898189-9-2.png146.62 KBrecrit
#9 facet_links_should_be-2898189-9--test-only.patch1.53 KBrecrit
#5 facet_links_should_be-2898189-5.patch2.21 KBborisson_
#5 facet_links_should_be-2898189-5--test-only.patch1.39 KBborisson_
#2 facets-pageless-links-2898189-2.patch826 bytesrecrit

Comments

recrit created an issue. See original summary.

recrit’s picture

recrit commented
Status: Active » Needs review
StatusFileSize
new facets-pageless-links-2898189-2.patch826 bytes

The attached patch removes the page parameter from the facet links.

recrit’s picture

recrit commented
Issue summary: View changes
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Should've been fixed a year ago with: #2704669: Remove page parameter from urls.

So we should write a new test to actually prove this behavior so we can't run into regressions again.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new facet_links_should_be-2898189-5--test-only.patch1.39 KB
new facet_links_should_be-2898189-5.patch2.21 KB

This adds a test. Test only should fail.

Status: Needs review » Needs work

The last submitted patch, 5: facet_links_should_be-2898189-5.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Either the test is insufficient or the code added here wasn't needed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Actually, the test here proves that the code was not needed. In what scenario does this still happen? The scenario you describe is being tested in the new test and that works without your code. ($result_get_params is already a clone of $get_params and that shouldn't have a 'page' anymore)

In the same method, we already do this:

    // When adding/removing a filter the number of pages may have changed,
    // possibly resulting in an invalid page parameter.
    if ($get_params->has('page')) {
      $current_page = $get_params->get('page');
      $get_params->remove('page');
    }

Test is still valuable though.

recrit’s picture

recrit commented
StatusFileSize
new facet_links_should_be-2898189-9--test-only.patch1.53 KB
new facets-debugging-2898189-9-2.png146.62 KB

I am seeing the issue occur starting with the 2nd link in the facet block. The first link is correct with no "page" parameter. See the debugging image attached.

I attached a new test that checks both content types in the facet block. The thinking that the second link in the block should fail. My local simpletest environment is having issues so posting for d.o to run.

recrit’s picture

recrit commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: facet_links_should_be-2898189-9--test-only.patch, failed testing. View results

recrit’s picture

recrit commented

Confirmed: failed on for $this->assertFalse(strpos($this->getUrl(), 'page=1')); on /var/www/html/modules/contrib/facets/tests/src/Functional/UrlIntegrationTest.php:203

recrit’s picture

recrit commented
Status: Needs work » Needs review
StatusFileSize
new facet_links_should_be-2898189-13.patch2.34 KB

Original patch with the #9 tests.
There could be a cleaner way to do this but this is a fail safe when creating the url for the link.
It appears that some code in the loop resets $this->request->query. I suspect that it is the recursion of the children call even though I am not using a hierarchical facet:

      // Sets the url for children.
      if ($children = $result->getChildren()) {
        $this->buildUrls($facet, $children);
      }

After this buildUrls() call in the loop it would set the $this->request->query's page back with the code at the end of the method:

    // Restore page parameter again. See https://www.drupal.org/node/2726455.
    if (isset($current_page)) {
      $get_params->set('page', $current_page);
    }
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Ok, this doesn't look very clean, but the test confirms that it works. So I'm going to believe the test. Thanks!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed, thanks!

  • borisson_ committed 64c5602 on 8.x-1.x authored by recrit 
    Issue #2898189 by recrit, borisson_: Facet links should be page-less

Status: Fixed » Closed (fixed)

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

chuchunaku’s picture

chuchunaku commented

Just an FYI. The patch for #13 removes the page parameter from the facet links but the patch for https://www.drupal.org/project/facets/issues/2726455 restores the page parameter back. As a result, the issue described in this ticket has re-appeared.

Both patches were merged and appear to compete with each other. https://cgit.drupalcode.org/facets/tree/src/Plugin/facets/url_processor/...

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented

I'm getting the page parameter in my facet URLs again. Has this been re-addressed since ChuChuNaKu's comment a year ago?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I don't think so, no. Could you open a new issue?