Comments

Beloa created an issue. See original summary.

borisson_’s picture

This sounds like a good idea, I'd be happy to accept a patch that does this.

waluigi’s picture

Assigned: Unassigned » waluigi

I'll give it a try :)

waluigi’s picture

Status: Active » Needs review
FileSize
1.38 KB

I've created a patch for this issue including a new function which replaces '#theme' => 'facets_result_item' in buildResultItem() with dynamically generated template suggestions.

It is pretty similar to the patch in https://www.drupal.org/project/facets/issues/2896202, but optimized for the item itself and not for the list.

Since this is my first contribution, I'd appreciate any feedback :)

Best Regards,
Nicolas

Status: Needs review » Needs work

The last submitted patch, 4: facets-theme-hook-result-items-2950094-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

waluigi’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
3.03 KB

The tests checked the theme name against the hardcoded 'facets_result_item' in buildLinkAssertion() in WidgetTestBase and not against the dynamically generated theme name.

Now it is also dynamically generated.

Moreover I have created a test named testThemeHookSuggestion() to check if the generated theme name is correct. This test checks both, the item name and the list name.

Best Regards,
Nicolas :)

Berdir’s picture

+++ b/src/Widget/WidgetPluginBase.php
@@ -238,7 +238,7 @@ abstract class WidgetPluginBase extends PluginBase implements WidgetPluginInterf
     $count = $result->getCount();
     return [
-      '#theme' => 'facets_result_item',
+      '#theme' => $this->getFacetResultItemThemeHook($result->getFacet()),
       '#is_active' => $result->isActive(),
       '#value' => $result->getDisplayValue(),

Using __ in #theme is a pretty rarely used feature and has some limitations, for example twig debug doesn't see it. That's why it doesn't work on views templates, see #2118743: [PP-3] Twig debug output does not display all suggestions when an array of theme hooks is passed to #theme.

As you can see there, some people are arguing to deprecate that feature.

The standard way to provide suggestions is to implement hook_theme_suggestions_HOOK(), see for example node_theme_suggestions_node()

That's going to make testing it harder though, as you need to actually implement such a template, access a page and make sure that it is used. We did that for example in #2965637: Ensure that the style templates are added at the end of suggestions

Berdir’s picture

You might want to give @borisson_ a chance to comment on whether he would even require such a test before going down that path :)

borisson_’s picture

Yeah, if we implement that hook - adding the tests isn't needed for the reasons mentioned in #7. Thanks so much for helping out here @Berdir!

dermario’s picture

Status: Needs review » Needs work

@waluigi maybe these steps help you to follow the advices from @Berdir and @borisson_:

Revert the changes you've made on your local.
Implement a new function called facets_theme_suggestions_facets_result_item(&$variables) { ... } in facets.module, that returns an array of possible theme suggestions. All required information should exist in $variables array. This hook function should be found and called automatically if a facet_result_item is themed.
As mentioned above you could have a look in here: https://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fu...

Good luck ;)

waluigi’s picture

Thank you for your input.

I've created a new patch including a new function in the facets.module, as @dermario described it.

Unfortunately, I had some issues with the base hook. As long as the facets_theme['facets_result_item'] has a base hook, it displays the default theme name (facets-result-item.html.twig) twice. Since I haven't seen any other conflict when I removed it, I left it that way.
I added two screenshots of the twig debugger so you can see what I mean.
with base hook:
twig debug
without base hook:
twig debug
It seems that the base hook was added in this issue: https://www.drupal.org/project/facets/issues/2894147

I am unsure if this is still in use or if we can leave the base hook removed.

I'd appreciate any feedback :)

Status: Needs review » Needs work

The last submitted patch, 11: facets-theme_hook_suggestions-2950094-11.patch, failed testing. View results

waluigi’s picture

The test most probably failed because the variable 'variables' was passed by reference to the facets_theme_suggestions_facets_result_item() function. Therefore I removed this and created a new patch and interdiff.

waluigi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: facets-theme_hook_suggestions-2950094-13.patch, failed testing. View results

dermario’s picture

Status: Needs work » Needs review
borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

Crediting @dermario and @Berdir for their feedback / help on getting this issue resolved.

  • borisson_ committed 10f4cf2 on 8.x-1.x authored by waluigi
    Issue #2950094 by waluigi, Berdir, dermario: add template suggestions...
borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again!

Status: Fixed » Closed (fixed)

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