Problem/Motivation
Currently, the module uses an InboundPathProcessor to listen on every incoming request, and if the request path matches one of the active facet source path, alter the path to pretend that it's in fact occuring on the facet source path. This has a lot of drawbacks, some of then fixed, some of them with pending patches, some of them not reported in the issue queue yet. Listing a few of them as examples:
- #2862038: Not working with Redirect because redirect thinks that all the requests are on the same path. The fix that went there disables redirect completly for those path, which is more than what's really needed and will prevent some normalization from happening when it'd be kinda nice to still keep it: trailing slash removal, alias normalization, ...
- #2897756: Views pager not working on pretty path because Views tries to regenerate a path and has no data to do it correctly
- #2779971: Pretty Paths for page manager routes with parameters. because the parameters are lost
- #2820741: Translation problem because language prefix doesn't work by default due to using "base:"
- #2897757: Could not create an alias for pretty path because the alias normalization loses the filters
- Not reported in the issue queue yet: language switcher links are broken (leads to the unfiltered search), links for search with facets in a menu are translated into unfiltered search links, links generated for facets that have an alias doesn't correctly use the alias, and probably quite a few more
Those issues can probably all be fixed individually, but I took it as a symptom of a bigger problem with the module architecture, and thus would like to propose a revamp.
Proposed resolution
Instead of having an inbound path alter service, add a route subscriber, that adds an additional parameters to all the facet source routes ; make that parameter receive the whole / parts ( a bit tricky due to #2741939: Cannot use a / in route parameter, workaround included in the patch ) ; use that parameter in the url processor plugin instead of trying to parse the request path.
Pros:
- All of core and contrib correctly deals with route parameters, so all of the above bugs get fixed magically.
- Performance wise, the facets source are only instantiated when the router is rebuilt, instead of every incoming request
- A lot less path processing, Symfony handles the route by itself, and hands us a properly formatted filter query string
Cons: Until the core bug is fixed, this needs a kinda ugly workaround in the RouteSubscriber, which will imply a cap on the number of active facets.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2930145-8-drop-pathprocessor.patch | 9.93 KB | strykaizer |
Comments
Comment #2
DeFr commentedComment #3
strykaizerHaven't tested yet, but I'm defenitly open to refactor this, as I agree using InboundPathProcessor is not ideal.
I'll try to review and test this asap.
Thanks for your work!
Comment #4
DeFr commentedPart of the patch from #2820741: Translation problem were incorporated in the previous patch, removed them here.
This also conflicts with #2924492: url_processor Not compatible with latest Facets UrlProcessorPluginBase, so integrated it in the new patch.
Comment #5
NerOcrO commentedSounds good for me!
Comment #6
strykaizer@Nerocro: can you confirm you thoroughly tested this patch? If not, please leave it to needs review, as this is a big change, and will need good testing of all features before we can rtbc
Comment #7
strykaizerNow that #2924492: url_processor Not compatible with latest Facets UrlProcessorPluginBase is in, needs re-roll for latest dev
Comment #8
strykaizerReroll added.
in the BuildUrls call in the FacetsPrettyPathsUrlProcessor.php we do not check parameters from the request anymore.
This allows users to create pretty path urls for a specific filter, regardless on which url they are.
Did some manual testing, and this looks good. Haven't tested multi language etc, but I suspect this approuch to be much cleaner then what we did before.
Comment #9
b_sharpe commented#8 working well for me, fixed a problem where ANY path was able to receive the filters and we were getting DDoS'd by bot trying to crawl search paths :D
Comment #11
strykaizer