When a Views display uses a contextual filter that uses a slug in the path (e.g. /node/%node/stuff), the results from getPath() and getUrl() are garbage and lead to issues when used. There was already acknowledgement of this shortcoming in one form in #2842557: Search API sorts doesnt work with contextual filters, but it only addressed the implementation of isRenderedInCurrentRequest().

Original issue (QueryString processor causes 404 for Views page displays that have upconverted contextual filter)

This one caused my head to hurt running it down. :)

Say you have a facet whose source is a Views page display. The path for this display includes a contextual filter AND the slug makes use of automatic upconversion, e.g. /user/%user/stuff.

The QueryString processor tries to build URLs by creating a request from the facet source's path.

    if ($facet->getFacetSource()->getPath()) {
      $request = Request::create($facet->getFacetSource()->getPath());
    }
    ...
    $requestUrl = Url::createFromRequest($request);

When creating a URL from the request, this blows up because the route enhancer that converts the parameter to the full entity barfs (%user is not a valid user ID). Since it can't load the entity, the ParamConverterManager throws an exception that in turn causes a 404 to be issued.

Comments

kevin.dutra created an issue. See original summary.

kevin.dutra’s picture

I'm not sure what the best way to fix this is.

Maybe the ViewsPage plugin out of the SearchAPI should simply not return a path if a contextual filter is in play. That would cause the current request to be used, which has a valid path, but I'm not sure that it's really how the getPath() method is supposed to be used, so maybe the change has to come within the QueryString processor.

borisson_’s picture

This is an interesting problem. No idea what the best place for a fix is in this case.

kevin.dutra’s picture

Title: QueryString processor causes 404 for Views page diplays that have upconverted contextual filter » Views display plugins with contextual filters produce invalid paths/URLs
Project: Facets » Search API
Component: Code » Facets
Assigned: Unassigned » kevin.dutra
Issue summary: View changes

After digging some more, I came across this issue: #2842557: Search API sorts doesnt work with contextual filters. There the default implementation of isRenderedInCurrentRequest() is overridden for the Views related display plugins to switch from path comparison to route name comparison. So there's acknowledgement there that the contextual filters are mucking things up with paths, but the patch only resolves the path problem for one specific use case. In any case, that does lead me to think that this should be dealt with within the realm of SearchAPI, so moving this issue there.

kevin.dutra’s picture

Assigned: kevin.dutra » Unassigned
Status: Active » Needs review
StatusFileSize
new720 bytes

Here's my initial thought. When a contextual filter is being used, just don't pass along the bogus path.

borisson_’s picture

Status: Needs review » Needs work

I would love to expand the comment to add something about the why.

So something like: Recreating a link when a contextual filter is used in the display's path is not possible. So instead we return NULL, which forces most implementations to use the current request's path instead.

The code looks solid though, do you agree with the change in the comment?

kevin.dutra’s picture

Status: Needs work » Needs review
StatusFileSize
new815 bytes
new720 bytes

Sounds good to me; better to be more clear so there's less chance of a regression being introduced later on down the road.

Status: Needs review » Needs work

The last submitted patch, 7: contextual-filter-paths-2913688-7.patch, failed testing. View results

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

In that case, this looks ready to me. The tesbot has been flaky all day so the composer require failure shouldn't be something to worry about.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: contextual-filter-paths-2913688-7.patch, failed testing. View results

kevin.dutra’s picture

Status: Needs work » Reviewed & tested by the community

Good to know, cancelled the retest request. Moving back to RTBC.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

cancelled the retest request

I don't think that's what he wanted to say, re-testing is still a good idea – even if there isn't a functional change compared with #5.

Anyways, the patch looks good and the reasoning is sound, so: committed. Thanks, everyone!

Status: Fixed » Closed (fixed)

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