Problem/Motivation

If the search api view is using contextual filter which uses uri instead of exposed filter which uses query param then the facet URL formed are not correct.

Steps to reproduce

  • Create a search API view page.
  • Add a contextual filter for full-text search.
  • Add a facet block to the view page and use the facet link.

Actual behaviour

  • Search page URL: /search
  • Search result page URL: /search/search term here
  • Facet URL: /search?filter_key[0]=facet_id:Applied facet term

Expected behaviour

  • Search page URL: /search
  • Search result page URL: /search/search term here
  • Facet URL: /search/search%20term%20here?filter_key[0]=facet_id:Applied facet term

Proposed resolution

  • Either use views argument to form the URL
  • Or just use the current path.

Remaining tasks

  • Agree on the apporoch.
  • Create a patch.
  • Review.
  • Commit.
  • Rejoice

User interface changes

None

API changes

None, it is a bug fix.

Data model changes

None.

CommentFileSizeAuthor
#12 3204660-12.patch2.99 KBjibran
#12 interdiff-bea384.txt3.28 KBjibran
#9 Bildschirmfoto 2021-03-22 um 14.49.56.png18.48 KBmkalkbrenner
#6 3204660-6.patch3.24 KBjibran
#7 3204660-7.patch3.9 KBjibran
#6 interdiff-8e248a.txt2.44 KBjibran
#7 interdiff-8249ce.txt1.15 KBjibran
#2 3204660-2.patch768 bytesjibran
#7 3204660-7-test-only.patch3.1 KBjibran

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3204660-2.patch768 bytes

This patch goes with the second proposed resolution.

just use the current path

+++ b/src/Plugin/facets/facet_source/SearchApiDisplay.php
@@ -136,13 +136,6 @@ public function getIndex() {
-    if ($this->getDisplay()->getPath()) {
-      return $this->getDisplay()->getPath();
-    }

This change might break something but it is worth a try.

I was unable to find the best way to implement the first proposed resolution.
use views argument to form the URL

Status: Needs review » Needs work

The last submitted patch, 2: 3204660-2.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

The test fails are because the current path is not set correctly for the test.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented 
1) Drupal\Tests\facets\Functional\FacetsUrlGeneratorTest::testCreateUrl
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'route:view.search_api_test_view.page_1;arg_0&arg_1&arg_2?f%5B0%5D=owl%3Afuzzy'
+'route:user.login?f%5B0%5D=owl%3Afuzzy'

It seems like the path that is used in the test requires a login. That might be a bug in the tests that is revealed by the patch.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-8e248a.txt2.44 KB
new 3204660-6.patch3.24 KB

Fixed the test fails but fixing the test also includes #3191091: QueryString url_processor broken(ish) for Ajax Views.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new 3204660-7-test-only.patch3.1 KB
new interdiff-8249ce.txt1.15 KB
new 3204660-7.patch3.9 KB

Here are the tests for this issue.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Needs review » Needs work

I tested the patch locally. It broke a functionality.

It is possible to show a facet block elsewhere on a page which is not the search view. In this case the generated URLs are wrong.
Without the patch it works fine and clicking on a facet entry elsewhere on the page takes you to the the search result view page.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
StatusFileSize
new Bildschirmfoto 2021-03-22 um 14.49.56.png18.48 KB

Maybe we should add a switch based on this condition. On the other hand I think that both issues targeted by the latest patch are caused by variations of the "master URL". So maybe we should simply check if the current path starts with the master URL. If yes we can use the current path.

mkalkbrenner credited neclimdul.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-bea384.txt3.28 KB
new 3204660-12.patch2.99 KB

How about this? I reverted the changes to existing test cases as they check for the case when the facet block is not placed on the search view.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

I'll manually test it tomorrow.

neclimdul’s picture

neclimdul
He/Him
commented

Excited about this being fixed! I've been running the master request for quite a while now without issue so definitely RTBC from me on that part but I'm not immediately able to grok the rest of this so I'll see if i can make some time to review/test as well.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Needs review » Reviewed & tested by the community

I tested the rendering without source manually.

neclimdul’s picture

neclimdul
He/Him
commented

I don't know that this is a blocker because I have a stack of patches precariously stacked to make it work but the differences between this and #3191091: QueryString url_processor broken(ish) for Ajax Views (the SearchApiDisplay::getPath changes) seem to break views ajax paths on the site I need this for. The url gets rewritten to the /views/ajax path and refreshes or history browsing end up with json.

I'll finish tracking down what is breaking and report back but wanted to drop a note about the limitation.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

@neclimdul your patch in #3191091: QueryString url_processor broken(ish) for Ajax Views will not be accepted as it breaks existing features.
You might need to add additional conditions based on this patch here and re-open your issue.

I think we should commit the current patch here as it seems to fix an issue without breaking others.
What do you think?

neclimdul’s picture

neclimdul
He/Him
commented
Status: Reviewed & tested by the community » Needs work

Sorry, I wasn't trying to push the other patch just the other patch didn't cause problems and the difference was somehow breaking my test site.

I think I have some details on how it broke now. The crux of the issue seems to be that isRenderedInCurrentRequest assumes the view is a page and only treats it as rendered if it is in the route parameters. If the view is a block, the display path returns null but the surrounding code falls back on the request everything still works fine in the normal page because that's still the right URL.

However in the always terrible edge case of ajax request, the rendering is happening under ViewAjaxController which hacks the current path but the Request path is still views/ajax and the fallback ends up building links to send the user to the views ajax controller.

This is a problem on this site because they embedding views in paragraphs so content editors can use standard tools for like hero headers around the view but the view needs to have all the normal "I'm a page" functionality because its the main content. I'll keep digging trying to get a better test case then "this super complicated page on a site" but maybe that will give a starting point.

PS. I didn't realize the other patch broke any features since the tests where green. Do you have something I could review as to what it broke?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

If the problem is in \Drupal\search_api\Display\DisplayInterface::isRenderedInCurrentRequest() then we have to fix it in search_api.

isRenderedInCurrentRequest assumes the view is a page and only treats it as rendered if it is in the route parameters.

I think we can add a check in \Drupal\search_api\Plugin\search_api\display\ViewsDisplayBase::isRenderedInCurrentRequest() that if it is an AJAX request then use views param from request else from route. Or we can just use request and ignore route params altogether.

I'll keep digging trying to get a better test case then "this super complicated page on a site" but maybe that will give a starting point.

I agree a test case would certainly help here. I have one question though, if the block is on some non view page then why it is not executing \Drupal\search_api\Plugin\search_api\display\ViewsBlock::isRenderedInCurrentRequest()? Is it because the block is used with AJAX-enabled view? I suspect #2769251: Contextual filter don't work with ajax in view might be the root cause.

neslee canil pinto’s picture

neslee canil pinto
he/him
Primary language English
commented

#12 worked out for me, thanks @jibran

patrickmichael’s picture

patrickmichael commented

#12 seems to work fine for me. That is with AJAX off.

mistrae’s picture

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

#12 worked fine for me

karishmaamin’s picture

karishmaamin commented

#12 worked for me as well

t.maquin’s picture

t.maquin commented

#12 worked for me ! Thanks

igonzalez’s picture

igonzalez commented

#12 Deactivation of filters is blocked. Don't work for me

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Version: 8.x-1.x-dev » 2.0.x-dev
Status: Reviewed & tested by the community » Needs review

Test again with 2.0.x and Drupal 9.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Needs review » Reviewed & tested by the community

It seems that this patch at least improves the situation. I think we should commit it as it is to have a better base for further patches.

  • mkalkbrenner committed 18f32ae on 2.0.x authored by jibran 
    Issue #3204660 by jibran, mkalkbrenner, neclimdul, PatrickMichael,...
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

noman_297’s picture

noman_297 commented

I have applied above patch on 8.x-1.8 version but that did not resolve the issue. I am using facets in a view rest export and with contextual filter Like group id and fulltext search, when I search a keyword I get the whole result but when I visit the facet url ie for blogs or events in my case. I got same all result not specific to the facets. My issue https://www.drupal.org/project/facets/issues/3251730 was closed as its a bit duplicate. my facet is like

{
"url":"http://sitename/api/v1/rest-custom-search/10/BIG%20UNDERSTANDING?IIM%20F...",
"values":{
"value":"event",
"count":2
}
}

10 is group id, iim_facet_filter is the facet filter I am applying. But when I visit the above path it's not giving me the event results but instead it gives me the whole result. Kindly someone help me out. Thanks

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Potential follow-up issue / regression: #3259123: ViewsDisplayBase::isRenderedInCurrentRequest() doesn't let the facet link to reset to original URL