I have a lot of product facets based on dimensions. Now only the indexed value is displayed.

Width:
* 7
* 10
* 15

It would be quite helpful to show the field suffix so it would list:

Width:
* 7 mm
* 10 mm
* 15 mm

Or in case if a price is indexed it could prefix the currency.

I can't find an option do this right now.

I would like to help but don't know where to start, would this be a separate plugin, widget or processor? Or an addition to the current default settings.

My first guess would be to extend the current list of options with a [] Display field prefix, [] Display field suffix. And fetch the field settings of the indexed field and show the prefix / suffix and extend the item template (facets-result-item.html.twig). I'm guessing this would be helpful for the checkbox, dropdown and links widget.

The most themable thing to do is to print prefix and suffix in separate values so they can be wrapped within html inside the facets-result-item.html.twig file. Making a processor limits this because the prefix and suffix would be part of the value.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ericclaeren created an issue. See original summary.

borisson_’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.74 KB

Adding it trough a processor sounds like the most simple idea. It's possible to add html trough that as well if we use | raw.

Status: Needs review » Needs work

The last submitted patch, 2: show_prefix_suffix_in-2909115-2.patch, failed testing. View results

ericclaeren’s picture

Wow that's quick :) Thanks for the work. I would not have mind to put in some of the work for this. I was looking for some advice how to take on this request.

The patch applied but didn't quite worked out as my feature request intended.

A few concerns:

  • Using {{ value|raw }} raises a security risk and the current patch is vulnerable to XSS. You could enter a script tag.
  • The original fields mapped with the facet already have a suffix and prefix (and possible translated ones) why wouldn't you re-use them based on the field instead of a custom one? This way you could get a translated version of the prefix and suffix.
  • Both prefix/suffix fields are required.
  • After saving the facet I got a notice: Undefined index: prefix in Drupal\facets\Plugin\facets\processor\PrefixSuffixProcessor->buildConfigurationForm() (line 52 of modules/contrib/facets/src/Plugin/facets/processor/PrefixSuffixProcessor.php).


Although it would be more work, a solution like count, which lives beside the value in the template would be a more stable and easier to change. Maybe like this. <span class="facet-item__value">{{ prefix }}{{ value }}{{ suffix }}</span>

In that way currently overridden templates would not break and both prefix and suffix would be escaped.

When not using a processor, what would be the best way to handle this? I won't mind to provide a patch.

borisson_’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
3.8 KB

Wow that's quick :) Thanks for the work. I would not have mind to put in some of the work for this. I was looking for some advice how to take on this request.

I was wondering if we could do this trough a processor and testing that was the same work as actually writing this implementation.

  1. Using {{ value|raw }} raises a security risk and the current patch is vulnerable to XSS. You could enter a script tag.
  2. The original fields mapped with the facet already have a suffix and prefix (and possible translated ones) why wouldn't you re-use them based on the field instead of a custom one? This way you could get a translated version of the prefix and suffix.
  3. Both prefix/suffix fields are required.
  4. After saving the facet I got a notice: Undefined index: prefix in Drupal\facets\Plugin\facets\processor\PrefixSuffixProcessor->buildConfigurationForm() (line 52 of modules/contrib/facets/src/Plugin/facets/processor/PrefixSuffixProcessor.php).
  1. We can make it non-vulnerable by using xss filtering (see patch)
  2. The field can have prefix/suffix, but it's not needed (taxonomy terms, ...) Extracting that could be possible, but then we should do #2851851: Facet source plugins should provide metadata for facet fields first
  3. We should fix that
  4. Should be fixed now - by fixing the default configuration

Although it would be more work, a solution like count, which lives beside the value in the template would be a more stable and easier to change. Maybe like this. {{ prefix }}{{ value }}{{ suffix }}

In that way currently overridden templates would not break and both prefix and suffix would be escaped.

It would break overridden templates anyway, because that output is not added. So we're dealing with a bc break no matter which way you slice it.

When not using a processor, what would be the best way to handle this? I won't mind to provide a patch.

In that case, we should change the Facet entity and adding those fields to it. Doing that makes this slightly harder to test, because it's not as isolated.

In any case, we need to write actual tests for this as well, but holding off on that until we agree on a solution

ericclaeren’s picture

Sorry for the delay, I might have an idea. Will try to work out an additional patch and test the solution in #2851851.

StryKaizer’s picture

Priority: Normal » Minor

Not sure if we should include this in core facets or not.
Implementation is trivial though (although this does not support plural/singular fallback).

Should we keep on writing processors for all use cases and add them to core facets, or should we document creating processors and point users for cases like this to the documentation?

borisson_’s picture

Because this also created a need to change the template, we might just commit the template change and the formatter should go in a seperate module. We shouldn't just keep on creating new processors.

ericclaeren’s picture

What's the direction? So a separate processor should be created for this?

My idea for value|raw is to put to replace the value with a renderable array, in that way a prefix / suffix could be added to this array as an additional child {{ content }} -> content.value, content.prefix etc., while the current template can still be the same. That would probably collide with this idea, do you think this would be an acceptable solution?

borisson_’s picture

Replacing the value with a renderable array might work, but I don't see why we should do that. That sounds like it would mean a big increase in our tests to cover all the cases we currently cover - but with the renderable array as well as the current string.

If it doesn't lead to the big test-change that I'm fearing it sounds like a good solution.

borisson_’s picture

Status: Needs review » Needs work

So I'd love to see the implementation in #9 to see how much of a refactor this leads us on. Not sure if I'm happy with it but I'd love to see how it looks like.

ericclaeren’s picture

Will try to work out a patch, but I haven't got experience with writing tests. Do you know a good starting point for writing them?

borisson_’s picture

A good first step would be to write the patch and put it trough the testbot to see what breaks.

Documentation for the tests can be found at https://www.drupal.org/docs/8/phpunit

ericclaeren’s picture

Thanks!

ericclaeren’s picture

Hi I have made 2 patches, one for the changes on facets, I wanted to rename the value variable to content, which is more inline with the use of renderable arrays, but did not rename it, because it would break current templates. The only use case now, I could imagine which would break this if someone has overridden the template and applied some custom filtering which expects a string instead of an array.

Also tweaked the processor a bit into a separate module. Will publish this module if that's okay with you. Also depending on if there changes would get into facets, if not, I need to adjust the build implementation back to the alteration of the result display value. And i'm still doubting if #prefix of a separate render child would be better, because the latter could be easier extracted in the template.

Ran the patch through testbot and didn't see any errors.

Please let me know what you think of this change and if it has a change for survival :)

Got the prefix/suffix from the source, so the changes in https://www.drupal.org/node/2851851 worked perfectly!

--- Edit

Back to the drawing board, maybe I could change the template to {{ content }} the content variable in a preprocess, so the rest of the widgets stay the same.

borisson_’s picture

The testfailures on php7 are expected:

-    '#value' => 'Llama'
+    '#value' => Array (...)

Looks like php5.x is not as happy though, so that is something we really have to look into - as long as php5.5+ is still supported by drupal core we have to do the same :(

ericclaeren’s picture

I'm getting the basic errors and fixed them, but don't understand how to tackle the #value' => Array (...) piece from the tests. I have changed it to an array so that's clear to me. But as I understand it the tests are returning strings, Should I alter the tests? But that feels like cheating.

Any ideas? I'm a bit stuck on what to do.

borisson_’s picture

Status: Needs work » Needs review
FileSize
671 bytes
4.42 KB

We change the data-structure of those values, so we should also update the tests. That looked to be pretty easy - so I did that. Locally this makes all unit tests green.

borisson_’s picture

Ah, I see why php5 isn't happy - there are string-typehints. We're still supporting php5.x, so we can't use those yet (#2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7) so we have to remove those type hints.

borisson_’s picture

FileSize
3.03 KB
4.12 KB
borisson_’s picture

Title: Show prefix/suffix in facet item » Refactor facet value to be renderable array instead of simple string for easier extendability

Retitling this issue to make it easier to see what exactly is going on in here.

ericclaeren’s picture

Shouldn't result implement RenderableInterface due to toRenderable()?

It was in #18 but removed in #20. The rest is looking very good. Adjusted the facet prefix/suffix processor to setRenderArrayProperty and everything seems to work fine. Thanks for helping out, sorry I'm taking so long to reply.

borisson_’s picture

Shouldn't result implement RenderableInterface due to toRenderable()?

I added RenderableInterface to ResultInterface, making it no longer needed for the implementation to also implement the same interface. This makes sure that others can implement their own resultInterface while toRenderable will still work.

+++ b/src/Result/ResultInterface.php
@@ -3,11 +3,12 @@
-interface ResultInterface {
+interface ResultInterface extends RenderableInterface {
ericclaeren’s picture

Ah missed that, that's indeed better!

StryKaizer’s picture

Status: Needs review » Needs work
  1.   public function setRenderArrayProperty($name, $value) {
        $this->renderArray[$name] = $value;
      }
    

    Here we introduce a facet-specific way to alter the render array.
    I'd prefer to keep this similar to what core does. People should implement hooks (in this case e.g. hook_preprocess_facets_result_item) to alter render arrays.

    For the original example (pre/suffix), that module should implement this hook, which has the given facet in $variables available. Check if the processor is activated and do changes if necessary.
    I understand that this is more complex for developers, but I prefer this instead of inventing a custom setRenderArrayProperty function.

  2. +++ b/src/Result/Result.php
    @@ -177,4 +182,26 @@ public function getFacet() {
    +    $this->setRenderArrayProperty('value', [
    +      '#plain_text' => $this->getDisplayValue(),
    +      '#weight' => 0,
    +    ]);
    

    No need for the 'value' key, just return an array with #plain_text and #weight as keys.
    Edit: not sure if weight is required either, probably can be removed too if no keys are set anymore. We can't have multiple values anyway in 1 result.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
3.37 KB

This fixes both items in #25, should we also introduce a test that implements that hook and checks that the results are as expected?

ericclaeren’s picture

I understand, but how do you know a processor is active on a result item? Is that information available inside the #facet object?

The downside, this preprocessor should check this on every single item. Doesn't this have a performance impact? To check on every item if a processor is active and fetch the processor information?

StryKaizer’s picture

@ericclaeren I agree that this is not an ideal solution either. Actually I'm not a big fan of using the value to render pre/suffix.
For simplicity, maybe we should just add the {{ prefix }}{{suffix}} tags in twig and define getters/setters for them.

@borisson_ @ericclaeren opinions?

Not sure if we need the renderableinterface in this case. Its not really a bonus for us (as our renderable interface is really hard to alter anyway, and as eric states, prolly adds to much performance issues when doing so in a hook).
I checked core, and they do not use a render array for the #title value in Link.php either, which is a bit similar to our result value.

@borisson_ sorry for the confusion, and thx for your work ;)

borisson_’s picture

Status: Needs review » Needs work

Not sure if we should change the entity schema, I guess it is easier. Let's do that. This is an API break though, but we can still do that now - especially if we default it to ""

StryKaizer’s picture

Slept another night over this.
should we really support html wrappers for the pre/suffix? Is this a use-case many people need?

If we just say people should use the displayvalue to pre/append stuff, and do not allow html in our displayvalue, we keep things much simpler.
We support REST facets too, which probably want the displayvalue not rendered in html. I dont want to add the pre/suffix tags in the rest facets as a separate tag, but I dont mind being them part of the display value.

For the few users who really need separate htmlwrappers, they can use hook_preprocess_facets_result_item and do the pre/suffix logic in there, although I really doubt many will actually need separate wrappers.

opinions?

borisson_’s picture

Sure, that sounds like it's the best idea. I forgot about rest. So that means we can use this issue to add docs about that in readme.txt?

ericclaeren’s picture

Hi, sorry for the delay. My girlfriend is pregnant and a baby on the way takes up a lot of my spare time. ;)

I'm okay with not adding the renderable part, with #facet as a variable, it's possible to get the data in the same way that it's fetched from the processor. It's quite some additional work for somebody who needs it to add that inside a template preprocess, but just altering the plain text is a more performant and understandable solution.

Too bad for the work that is done by @borisson_. But it was a valuable lesson for me. Thanks for your help.

I will change the prefix/suffix processor to adjust the display data an put that on drupal.org. Will add project link to this issue when it's up.

ericclaeren’s picture

borisson_’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
912 bytes

Implemented the readme changes, like I said in #31.

borisson_’s picture

Title: Refactor facet value to be renderable array instead of simple string for easier extendability » Add prefix/suffix module to readme.md

  • borisson_ committed 0b4b761 on 8.x-1.x authored by ericclaeren
    Issue #2909115 by borisson_, ericclaeren, StryKaizer: Add prefix/suffix...
borisson_’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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