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.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | interdiff.txt | 720 bytes | kevin.dutra |
| #7 | contextual-filter-paths-2913688-7.patch | 815 bytes | kevin.dutra |
| #5 | contextual-filter-paths-2913688-4.patch | 720 bytes | kevin.dutra |
Comments
Comment #2
kevin.dutra commentedI'm not sure what the best way to fix this is.
Maybe the
ViewsPageplugin 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 thegetPath()method is supposed to be used, so maybe the change has to come within theQueryStringprocessor.Comment #3
borisson_This is an interesting problem. No idea what the best place for a fix is in this case.
Comment #4
kevin.dutra commentedAfter 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.Comment #5
kevin.dutra commentedHere's my initial thought. When a contextual filter is being used, just don't pass along the bogus path.
Comment #6
borisson_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?
Comment #7
kevin.dutra commentedSounds good to me; better to be more clear so there's less chance of a regression being introduced later on down the road.
Comment #9
borisson_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.
Comment #11
kevin.dutra commentedGood to know, cancelled the retest request. Moving back to RTBC.
Comment #13
drunken monkeyI 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!