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:

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.

Comments

DeFr created an issue. See original summary.

DeFr’s picture

Status: Active » Needs review
StatusFileSize
new11.86 KB
strykaizer’s picture

Haven'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!

DeFr’s picture

StatusFileSize
new10.78 KB
new4.13 KB

Part 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.

NerOcrO’s picture

Status: Needs review » Reviewed & tested by the community

Sounds good for me!

strykaizer’s picture

@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

strykaizer’s picture

Status: Reviewed & tested by the community » Needs work
strykaizer’s picture

Status: Needs work » Needs review
StatusFileSize
new9.93 KB

Reroll 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.

b_sharpe’s picture

#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

  • StryKaizer committed 81bcdbc on 8.x-1.x
    Issue #2930145 by DeFr, StryKaizer: Change path processing approach:...
strykaizer’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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