Allow other modules to alter the url built for the facet item.

Adding support to add query parameters directly as well (non f[*] query params).

We should add test-coverage for this as well. Probably by introducing a new testmodule. The idea is good though.

I slightly adjusted the approach to use events instead of hooks, since that's the direction search api and core are moving in. I've also added a test to cover this use case.

Let's see what testbot thinks about it.

This is looking super super solid already. I am super grateful that you took on this issue @legolasbo. I would've just implemented the suggested hook instead of using this as an event. Thank you for making sure we use modern best practices.

I mentioned to change private to protected a few times in the review here. I think the general consensus is to not make things protected because that way other can easily overwrite this.
On the other hand - this is an Event, not a normal class. Would someone even be overwriting/extending that? I think we could also ignore that and keep them as private and instead make the class final. That'd also work for me.
That might even be better.

1. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +class QueryStringCreated extends Event {
   

   I actually like this event name, in contrast to #9. How I understand the naming convention we should use for events it should be the name of what just happened, not what action is possible to do when it is thrown.

   So +1 from me.

2. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  private $getParameters;
   

   /s/private/protected/

3. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  private $filterParameters;
   

   /s/private/protected/

4. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  private $facetResult;
   

   /s/private/protected/

5. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  private $activeFilters;
   +  /**
   

   /s/private/protected/ Also needs a newline after it.

6. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  private $facet;
   

   /s/private/protected/

7. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  public function __construct(ParameterBag $getParameters,
   +                              array $filterParameters,
   +                              ResultInterface $facetResult,
   +                              array $activeFilters,
   +                              FacetInterface $facet
   +  ) {
   

   Not sure what the indentation rules are when splitting a constructor over multiple lines. But this looks odd. It could be that this is correct though. In that case please ignore me.

   I think the readability doesn't suffer too much when this is on one line.

8. +++ b/src/Event/QueryStringCreated.php
   @@ -0,0 +1,147 @@
   +  public function getGetParameters() {
   

   I agree with @joshi.rohit100 that this looks really weird because of the double get. Is getQueryParameters better? (we'd have to change the underlying property as well).

9. +++ b/src/Plugin/facets/url_processor/QueryString.php
   @@ -27,14 +30,33 @@ class QueryString extends UrlProcessorPluginBase {
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   

   Needs an {@inheritdoc}

Didn't mean to change status. :/

This should do it :)

I forgot about this issue and just wanted to commit it, but it no longer applies.

Just rerolled the patch 2970987-12.patch

Committed and pushed, thanks!

At the moment it is not possible to alter the filterParams through the eventSubscriber.
It's only possible to add extra params to the result_get_params because it is an object.
If you want to remove some filterParam it will be overridden by the following line.
$result_get_params->set($this->filterKey, array_values($filter_params));

#18 was fixed in a followup isse FYI: #3182988: The QueryStringCreated event dispatcher does not work