Make it easier to programmatically generate facet links

If we can, we should do this. I think it's probably a new method on the facet source plugin's. That'd be a good thing to do. We should load and execute the view but move it to a method on that plugin. That'd be the easiest way to implement this correctly I think. Going to float this by Nick and Jimmy to see if they agree with that approach.

I attach a patched with a suggestion on how we could start this.

Extend the url processor with getFilterString method to get the value string of the facet

  public function getFilterString($raw_value) {
    return $this->urlAlias . $this->getSeparator() . $raw_value;
  }

and this to add/remove specific query values for a raw facet value.

public function getFilterParamsForRawValue(FacetInterface $facet, $raw_value, $filter_params = [], $active = FALSE);

and than we could add a face source method to use these 2 methods

Doing this will break the API, so we should do this before we tag the first beta.

Mainly docs changes. Going to ask @StryKaizer for a review, as he will be able to look at this from the pretty paths perspective as well.

1. +++ b/src/Plugin/facets/url_processor/QueryString.php
   @@ -148,6 +120,54 @@ public function buildUrls(FacetInterface $facet, array $results) {
   +  public function getFilterParamsForRawValue(FacetInterface $facet, $raw_value, $filter_params = [], $active = FALSE) {
   

   public => protected

2. +++ b/src/Plugin/facets/url_processor/QueryString.php
   @@ -148,6 +120,54 @@ public function buildUrls(FacetInterface $facet, array $results) {
   +          $filter_params[] = $this->getFilterString($parent_ids[0]);
   

   Can we return an url object instead if possible?

   Facets pretty paths does not use GET params, thus will only be able to generate URL objects.

   We can rename this function to getFacetItemUrl if so.

3. +++ b/src/UrlProcessor/UrlProcessorInterface.php
   @@ -58,4 +58,32 @@ public function getFilterKey();
   +  /**
   +   * Get the query params array for a raw value of a facet.
   +   *
   +   * @param \Drupal\facets\FacetInterface $facet
   +   *   The facet to generate query params for.
   +   * @param string $raw_value
   +   *   The raw value of the facet result.
   +   * @param array $filter_params
   +   *   The query params (i.e current query on a search page) to be used
   +   *   for setting the correct filter.
   +   * @param bool $active
   +   *   If the $raw_value should be considered as already active.
   +   *
   +   * @return array
   +   *   An array of query params to generate a url.
   +   */
   +  public function getFilterParamsForRawValue(FacetInterface $facet, $raw_value, $filter_params = [], $active = FALSE);
   

   I think this is not required for the interface.

Fixes most of the remarks of #6.

In that issue was added a @todo pointing to this issue.

The patch in #2844955 pulls of this trick by faking a search result.

For the record, this is exactly what ERFL does too for its formatters.

Here is an example on how to generate a facet link in code.

use Drupal\facets\Result\Result;

// Your data
$facet_id = 'tags'; // Machine name of facet to use
$raw_value = 35; // Raw value of result you want to create a link for.

// Create a Result object which can output an Url for given facet.
$facet = \Drupal::entityTypeManager()->getStorage('facets_facet')->load($facet_id);
$url_processor = \Drupal::service('plugin.manager.facets.url_processor')->createInstance($facet->getFacetSourceConfig()->getUrlProcessorName(), ['facet' => $facet]);
$result = $url_processor->buildUrls($facet, [new Result($raw_value, '', 0)])[0];

// Output.
$url = $result->getUrl();

When all you need is linking an entity reference field to the facet search, I suggest using the ERFL module as mentioned above which does basicly the above code, and provides field formatters for your entity references.

The code from #8 does not really adress this issue, not sure if we need this refactor.

For those who don't know yet, ERFL Beta 3 comes with a second formatter that outputs a raw URL and nothing else. It's intended to be used with Views to enable you to rewrite any field as a link to a facet, not just the entity reference field it comes from.

The example in #11 is a valid one, except that if you have the facets already processed, will give you the first version of url, which may contain other filters (I was working on improving facets_system_breadcrumb_alter)

I think at least we have to remove the @todo from here #2717537: Breadcrumbs support also I think we can improve the code there, and I don't think adding ERFL as a dependency is a good idea, but not really sure.

What do you think?

Well, this makes sense. If more people need this functionality, the code above is not that clean to re-use.
Lets create an utility service for this.

Attached is an utility service to simplify this process.

You can generate an Url object for a facet link using following code, where 'tags' is the facet id, and 35 is the raw value (term id in this case)

$url = \Drupal::service('facets.utility.url_generator')->getUrl('tags', 35);

ok, thanks. i will open a new issue for improving the code from #2717537 based on this

I don't know how to write the test - this is the current dump of what I have.

@dragos-dumi noticed that this helper function returns the active facets too
Lets fix/test this

Test! Green test! Woo.

Back to needs work per #21

+++ b/src/Utility/FacetsUrlGenerator.php
@@ -0,0 +1,56 @@
+    $result = $url_processor->buildUrls($facet, $results)[0];

I hadn't patched ERFL yet, but I realized about a week ago that this has some code smell. Core typically uses reset() in these situations, which seems like the safer thing to do instead of risking an undefined index warning. There doesn't seem to be any guarantee that a UrlProcessor won't remove Results and then return an empty array.

I modified #22 to use reset() and to do a basic check for a Result before returning.

Sorry, status.

Back to Needs Work again.

@dragos-dumi noticed that this helper function returns the active facets too
Lets fix/test this

I think I would prefer to test instead of fix. If a person clicks an ERFL link within a result displayed on a search result page, I think they would expect it to narrow the results, not reset them to only be filtered by that facet. When I demoed ERFL to my DUG this was the behavior they expected and were pleased to see happen. Thoughts?

I tested this by temporarily updating ERFL's formatter plugin base to use the new service. It was working well. The functionality gets a +1 from me.

I thought about the patch some more after posting #26. Assuming you decide not to remove the already-active facets from the URL, does this really need a test? Adding the active facets is a function of the URL processor that is configured. So is there a point to testing it at the service level? The result will vary depending on what the processor does.

I did a few more things:
1. Removed unused classes from the test.
2. Fixed an "Only variables should be passed by reference" error that was a result of the change to reset() in #24.
3. Added an InvalidArgumentException if an invalid facet ID is passed to createUrl().
4. Added a test for the exception.

By the way, from the perspective of someone who is consuming this API, I don't mind passing a Facet object instead of a facet ID. In that case, most of what I added in this patch would be irrelevant. I don't know if there was a reason for requiring the ID as a parameter instead. Of course, ERFL already has to load Facet objects to do its thing, so I may be biased because it's not a big deal for me.

I think I would prefer to test instead of fix. If a person clicks an ERFL link within a result displayed on a search result page, I think they would expect it to narrow the results, not reset them to only be filtered by that facet. When I demoed ERFL to my DUG this was the behavior they expected and were pleased to see happen. Thoughts?

We should keep it as an option I think, I would prefer the current implementation being in ::createUrlWithCurrentFilters and a new one in ::createUrl.

I thought about the patch some more after posting #26. Assuming you decide not to remove the already-active facets from the URL, does this really need a test? Adding the active facets is a function of the URL processor that is configured. So is there a point to testing it at the service level? The result will vary depending on what the processor does.

The result will vary on what the processor does indeed. But since this will be an entrypoint used by a lot of custom code we want to be 100% sure how it works with the current implementation of the query string url processor. Breaking that one isn't really an option at this point.

By the way, from the perspective of someone who is consuming this API, I don't mind passing a Facet object instead of a facet ID. In that case, most of what I added in this patch would be irrelevant. I don't know if there was a reason for requiring the ID as a parameter instead. Of course, ERFL already has to load Facet objects to do its thing, so I may be biased because it's not a big deal for me.

I know the additional load can seem like overkill but this keeps the api simplest for other usecases, I think. I'll discuss this later today.

I also made some changes to the code, I like the Exception but they should never use additional API calls, changed that out. Move the test to the correct namespace as well.

NW per #29.

In theory this should work.

Now with the patch.

I like the Exception but they should never use additional API calls, changed that out.

Makes sense.

Sorry I forgot that docblock.

How about this? I eliminated some code repetition.

@StryKaizer had the same idea earlier - thanks @dcam! I haven't written a test or manually tested this so not sure if it will work.

Yeah, I took a shot at writing a test, but I couldn't figure out how to set an active facet since the ID is passed and the facet object is reloaded from storage. I figured someone more familiar with writing tests for the module might know.

I now added a test to test all this behaviour, but it looks like it's not working properly.

The problem was here *points to self*. I uploaded the wrong patch. New patch incoming!

Looks like the testbot is not picking up on the new testfile, it should be red - not green. Now to figure out why that is.

I think the namespace was incorrect, hoping for testfails now.

On #2908937: Dependend Facets don't reset after Conditions are not met anymore. we need to overwrite the facet url so that removes another facet filter key from url. Could this be possible also with an url processor method?
Another situation this kind of method would be helpful is in facets_summary reset link processor, which now removes the query parameters, but that would not work for pretty facets url processor

Working on this

Beta blocker again, changed the URL processor interface to allow building links with multiple items, as required by e.g. breadcrumbs.

The syntax for the helper service changed a bit to allow users to generate links with multiple facets being active.

Tests will probably fail, needs to be fixed.
Pretty paths will need to be fixed too (but I got a working version on my local machine).

Example code

// URL object for 1 filter
$url = \Drupal::service('facets.utility.url_generator')->getUrl(['tags' => [7]]);

// URL object with multiple filters
$active_filters = ['tags' => [5, 7], 'color' => ['blue']];
$url = Drupal::service('facets.utility.url_generator')->getUrl($active_filters);

The getUrl method accepts an extra boolean to disable currently active filters, when using this method in a request which already has active filters.

Should work now, but tests still WIP
Changing category to Task as we really need this in for many reasons (the part which allows you to generate an url for a specific combination that is)

Still 2 remaining fails locally - but I don't know why.

Fixed the unit test + cs fixes all around.

Still working on this, buildUrls needs to use machinename=>url_alias conversion too

We should still write a test which checks if a facet with a different url_alias then the facet id generates a correct url.
This was still failing now (diff attached fixes this).

Tests should be green again now.

Rebased to get it back to work, should we commit this as-is @StryKaizer??

Fine for me

Ok, thanks!

Committed and pushed. This was the last of our beta blockers, I'd suggest that we go trough the entire queue again to see if any of the open issues are still blockers - if they are not we can tag the first beta release.

Congrats on clearing out your beta blocker queue!