Facets summary does not take hierarchical facets into account in building a list of items.
And on the other hand, the DefaultFacetManager does not take into account that hierarchical facets could have already been built by DefaultFacetsSummaryManager.

Attached patch solves these two issues with facets summary (at least for me).

CommentFileSizeAuthor
#24 facets_summary_breaks-2846294-24.patch13.14 KBborisson_
#24 interdiff-2846294.txt1.88 KBborisson_
#22 facets_summary_breaks-2846294-22.patch12.63 KBborisson_
#22 interdiff-2846294.txt1.07 KBborisson_
#20 facets_summary_breaks-2846294-20.patch12.63 KBborisson_
#18 facets_summary_breaks-2846294-18.patch12.61 KBborisson_
#14 facets_summary_breaks-2846294-14.patch13.79 KBborisson_
#14 test-only.patch9 KBborisson_
#11 2846294-10.patch3.61 KBjummonk
#9 facets-summary-hierarchical-9.patch4.79 KBjummonk
#8 facets-summary-hierarchical-8.patch4.19 KBjummonk
#6 2846294-1-6.txt2.09 KBshylajaphp
#6 2846294-6.patch3 KBshylajaphp
facets-summary-hierarchical-1.patch3.18 KBjummonk

Comments

jummonk created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Active » Needs work

Thanks, these changes look good, sorry for taking so long in getting to this issue.

I have some nits to pick with the current patch but the approach is solid!

  1. +++ b/modules/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
@@ -146,6 +146,29 @@ class DefaultFacetsSummaryManager {
+   * Built result tree, taking possible children into account

    Needs a full stop. 

  2. +++ b/modules/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
@@ -146,6 +146,29 @@ class DefaultFacetsSummaryManager {
+   * @param $results
...
     /** @var \Drupal\facets\Result\Result $result */

    Should probably be array $results and needs a description as well. I think these are all results right? Should we should do \Drupal\facets\Result\Result[] $results.

    Doing so removes the need for the inline @var comment 

  3. +++ b/modules/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
@@ -146,6 +146,29 @@ class DefaultFacetsSummaryManager {
+  function builtResultTree($results) {

    We can typehint on array here. 

  4. +++ b/src/FacetManager/DefaultFacetManager.php
@@ -434,6 +434,13 @@ class DefaultFacetManager {
+            // Children could already be built by Facets Summary

    Let's expand this comment a little bit.

    Is this correct?
    If the facets summary block is shown before this block, the children might have already been built. In that case, we should add them here.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue tags: +Novice

Tagging as novice, the nits are mostly docs.

themic8’s picture

themic8 commented

Still reviewing, but this patch has fixed my problem as well.

Also, looks like the counts might be cached. They disappear from time to time. Looking at this now.

shylajaphp’s picture

shylajaphp commented
Assigned: Unassigned » shylajaphp

I am working on it.

shylajaphp’s picture

shylajaphp commented
Assigned: shylajaphp » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2846294-6.patch3 KB
new 2846294-1-6.txt2.09 KB

Hi,

I have fixed the issue as per above suggestions.

Added new patch and interdiff file.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue tags: +Needs tests

Thanks for those changes, this now needs an integration test to make sure this won't break again.

jummonk’s picture

jummonk commented
StatusFileSize
new facets-summary-hierarchical-8.patch4.19 KB

Would you consider this addition? Adding facet id and raw result as an extra property to each facet result item of the facets summary block, so custom modules can override facets-result-item template and allow custom theming depending on facet id and/or facet item value. See patch in attachment.

jummonk’s picture

jummonk commented
StatusFileSize
new facets-summary-hierarchical-9.patch4.79 KB

@shylajaphp, apparently your patch broke my patch.

You removed this necessary piece from DefaultFacetManager:

else {
            // Children could already be built by Facets Summary
            $children = $keyed_results[$current_id]->getChildren();
            if (!empty($children[$child_id])) {
              $child_keyed_results[$child_id] = $children[$child_id];
            }
          }

And I was so incautious to base my new patch on yours.
I've adapted my latest patch to include yours and now it works again.

shylajaphp’s picture

shylajaphp commented

Thanks jummonk .

jummonk’s picture

jummonk commented
StatusFileSize
new 2846294-10.patch3.61 KB

Patch #8 and #9 are introducing an extra property in the facet result item. As this is not really related to the original issue I'm removing this extra feature and will make another feature request for that.
The new patch is without that unrelated feature.

Status: Needs review » Needs work

The last submitted patch, 11: 2846294-10.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue tags: -Novice

Removing the novice tag, keeping the needs tests tag for an automated test-case. I'll give that a go later today.

Thanks for splitting up the feature request @jummonk!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new test-only.patch9 KB
new facets_summary_breaks-2846294-14.patch13.79 KB

Adds a test, if the test-only fails and the other patch passes we can commit this.

The last submitted patch, 14: test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: facets_summary_breaks-2846294-14.patch, failed testing.

jummonk’s picture

jummonk commented

@borisson_. I'm afraid you have the 'extra' which I moved to another feature request in your new patch. You better base your patch on #10 and not #8 or #9

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new facets_summary_breaks-2846294-18.patch12.61 KB

#11 + test from #14

Status: Needs review » Needs work

The last submitted patch, 18: facets_summary_breaks-2846294-18.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new facets_summary_breaks-2846294-20.patch12.63 KB

Reroll of #18

Status: Needs review » Needs work

The last submitted patch, 20: facets_summary_breaks-2846294-20.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2846294.txt1.07 KB
new facets_summary_breaks-2846294-22.patch12.63 KB

This should fix the test.

Status: Needs review » Needs work

The last submitted patch, 22: facets_summary_breaks-2846294-22.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2846294.txt1.88 KB
new facets_summary_breaks-2846294-24.patch13.14 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Fixed

Committed and pushed.

  • borisson_ committed 82e13a4 on 8.x-1.x 
    Issue #2846294 by borisson_, jummonk, shylajaphp: Facets summary breaks...

Status: Fixed » Closed (fixed)

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