Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Once #2625152: Implement a Hierarchical Entity Processor is in place, we should create hierarchical facets.
Lets start with taxonomy support first, then implement a pluggable system to support custom hierarchical structures (e.g. based on an entity reference field)
Comment | File | Size | Author |
---|---|---|---|
#81 | Screen Shot 2016-12-06 at 1.53.18 PM.png | 38.97 KB | ndrake86 |
#75 | implement_hierarchical-2807333-75.patch | 45.38 KB | borisson_ |
#60 | interdiff.txt | 1.63 KB | ndrake86 |
#60 | implement_hierarchical-2807333-60.patch | 32.56 KB | ndrake86 |
Comments
Comment #2
StryKaizerComment #3
StryKaizerComment #4
aspilicious CreditAttribution: aspilicious commentedTHNX! :D
Next week I'll test this is our D8 project.
Comment #5
borisson_Also attributing @aspilicious for this patch per feedback on irc.
Comment #6
StryKaizerComment #7
StryKaizerComment #8
StryKaizerComment #11
StryKaizerNow implemented a new plugin type "hierarchy", to allow users to create custom hierarchical logics (e.g. entity reference fields).
At this moment only taxonomy term is supported though.
Comment #12
StryKaizerComment #13
borisson_c/p remnant.
Of what is this an array, can we add a more descriptive annotation?
Is there an interface we can use to hint on instead?
This needs some docs.
Needs docs as well.
Is there a reason why we use MasterRequest instead of CurrentRequest here? I'm actually not very sure what the difference is. So maybe just ignore this
Nice that this is cached!
I'm not sure about subchilds, maybe grand_children makes more sense?
Nice that this is cached as well, but let's make this camelCase
This is a perfect comment!
I mostly looked at how this was built architecturally, but I don't see anything that I'd change, very cool!
Comment #16
StryKaizerCleaned up the default classes and added hierarchy info for themers
Comment #19
StryKaizerAddressed #13
Comment #20
StryKaizer- Made tests less red
- added indeterminate state for checkbox
- default hierarchical indentation css
Comment #21
aspilicious CreditAttribution: aspilicious commentedcss file is missing
Comment #22
StryKaizerParents getting enabled when childs get disabled is now configurable.
interdiff is for #16, ignore #21 ;)
Comment #23
StryKaizerSmall css fix added.
This functionality is now pretty much in a usable state for taxonomy term hierarchy.
We do not have any test coverage yet, though.
Some stuff testcoverage should check
- If hierarchical structures behave as plain structures when hierarchy is disabled
- Expanded/non expanded hierarchies
- parent enabling/disabling depending on setting
Comment #24
StryKaizerComment #25
StryKaizerSome small bugfixxes added
Comment #26
borisson_Overall this looks great, still needs tests and some battle-testing.
cleanup attached.
Comment #27
StryKaizerFix to make this php5.5 compatible added.
Comment #28
borisson_Is there a way we can do this just on the
li
class instead of doing this?can we change this to
return $this->widget_plugin_manager ?: \Drupal::service('plugin.manager.facets.hierarchy');
Most of our setters don't return $this, so let's remove this.
Comment #29
Alienpruts CreditAttribution: Alienpruts commentedPatch at #23 partially failed because I implemented https://www.drupal.org/node/2784349#comment-11707151 .
This is probably to be expected, but just wanted to warn you all the same :)
For the rest I can confirm it seems to work as expected, building a term tree and allowing the facet to filter on the term chosen.
Comment #31
borisson_Comment #32
marthinal CreditAttribution: marthinal commentedDone!
Comment #33
borisson_#28 still needs to be addressed.
Comment #34
borisson_Resolves #28.
This still needs tests, but I think we should probably get #2815187: Cleanup entities and write kernel tests for them in first, so we can add a kernel test as wel as integration tests.
Comment #35
borisson_back to NR
Comment #36
aspilicious CreditAttribution: aspilicious commentedIt works :).
For some reason I had to rebuild the facet before it appeared, but hey it works now :)
Comment #37
aspilicious CreditAttribution: aspilicious commentedA bug we found: When selecting "Always expand hierarchy", "Enable parent when child gets disabled" is also selected (wrong default value)
Comment #38
StryKaizerWell, its not really a bug. It depends on which widget you use, which default makes the most sense.
When all items are expanded using checkboxes, imo it makes the most sense when disabling a child, the parent is enabled (since the parent of an active child, by default, is rendered in "indeterminate" state) (not visible on your screenshot though).
When all items are expanded using links, imo it makes the most sense to disable parents too, when links are clicked.
Then again, if most people prefer the default value to be different, its fine by me ;) Its a setting anyway...
Comment #39
borisson_Fixed some docs.
Comment #41
borisson_Back to needs review, the fail was a random fail (AlreadyInstalledException)
Comment #42
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedAwesome work!
FYI: When using this patch in combination with the "current search" patch from #2735891, I no longer see the facet tree (only the root elements appear) and I get the following notice:
Comment #43
borisson_@mpp: we probably shouldn't worry about that yet. I think this one'll go in before the other one, especially because this is being used in development on a couple of sites already and we haven't really heard anything that isn't working about this.
Comment #44
Shreya Shetty CreditAttribution: Shreya Shetty as a volunteer and at Trigyn Technologies Ltd commentedIt works for me . But can i select all children on just select of a single parent and deselect all childrens on deselect of single parent . Is it feasible to achieve this kind of functionality
Comment #45
StryKaizer#44 It is possible that certain content is only tagged with parents. Selecting children automaticly would imply it needs to be tagged with a child too. Therefor, no.
However, you can prolly visually achieve this by using css if your dataset will always have a child tagged.
This will not be implemented in the plugin itself, as in my opinion, this makes the functionality not clear on how it behaves.
Comment #46
Shreya Shetty CreditAttribution: Shreya Shetty as a volunteer and at Trigyn Technologies Ltd commentedMay be we could give flexibility for the users to have this option . If we are implementing this feature i can start working on this . This will be a "good to have" feature in facets . I would rather like to hear from experts though
Yes only the tagged childrens should get selected automatically when the parent is selected. And only the tagged children is visible on facets otherwise the term is not visible whichever is not tagged in facets
Comment #47
borisson_You did hear from an expert, @StryKaizer is exactly that. Anyway, I think that is something we can discuss in a follow-up. The only thing that this needs before committing is an integration test that tests this behavior. Let's try to get that written so we can commit this.
Comment #48
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedI tested out the processor here and came across one problem: Using the "translate entity " processor to change the taxonomy terms into back into their labels I am seeing the following error:
Error: Call to a member function getSetting() on null in Drupal\facets\Plugin\facets\processor\TranslateEntityProcessor->build() (line 111 of /var/www/build/html/modules/contrib/facets/src/Plugin/facets/processor/TranslateEntityProcessor.php) #0 /var/www/build/html/modules/contrib/facets/src/FacetManager/DefaultFacetManager.php(297): Drupal\facets\Plugin\facets\processor\TranslateEntityProcessor->build(Object(Drupal\facets\Entity\Facet), Array) #1 /var/www/build/html/modules/contrib/facets/src/Plugin/Block/FacetBlock.php(82): Drupal\facets\FacetManager\DefaultFacetManager->build(Object(Drupal\facets\Entity\Facet)) #2 /var/www/build/html/core/modules/block/src/BlockViewBuilder.php(203): Drupal\facets\Plugin\Block\FacetBlock->build() #3 [internal function]: Drupal\block\BlockViewBuilder::preRender(Array) #4 /var/www/build/html/core/lib/Drupal/Core/Render/Renderer.php(376): call_user_func('Drupal\\block\\Bl...', Array) #5 /var/www/build/html/core/lib/Drupal/Core/Render/Renderer.php(195): Drupal\Core\Render\Renderer->doRender(Array, true) #6 /var/www/build/html/core/lib/Drupal/Core/Render/Renderer.php(151): Drupal\Core\Render\Renderer->render(Array, true) #7 /var/www/build/html/core/lib/Drupal/Core/Render/Renderer.php(574): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() #8 /var/www/build/html/core/lib/Drupal/Core/Render/Renderer.php(152): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #9 /var/www/build/html/core/lib/Drupal/Core/Render/Renderer.php(166): Drupal\Core\Render\Renderer->renderPlain(Array) #10 /var/www/build/html/core/modules/big_pipe/src/Render/BigPipe.php(527): Drupal\Core\Render\Renderer->renderPlaceholder('callback=Drupal...', Array) #11 /var/www/build/html/core/modules/big_pipe/src/Render/BigPipe.php(395): Drupal\big_pipe\Render\BigPipe->renderPlaceholder('callback=Drupal...', Array) #12 /var/www/build/html/core/modules/big_pipe/src/Render/BigPipe.php(137): Drupal\big_pipe\Render\BigPipe->sendPlaceholders(Array, Array, Object(Drupal\Core\Asset\AttachedAssets)) #13 /var/www/build/html/core/modules/big_pipe/src/Render/BigPipeResponse.php(41): Drupal\big_pipe\Render\BigPipe->sendContent('...', Array) #14 /var/www/vendor/symfony/http-foundation/Response.php(374): Drupal\big_pipe\Render\BigPipeResponse->sendContent() #15 /var/www/build/html/index.php(20): Symfony\Component\HttpFoundation\Response->send() #16 {main}.
It looks like everything should be working correctly based on the SC provided above but I am cannot see if I have a mis-configuration or if their is an issue with the processor and Solr as the backend.
Search-API -> Beta3
Search-API-Solr -> Beta1
Core -> 8.2.1
Facets -> 3d18651 (dev-master -1 at time of writing). I will debug some more but outside of this, which might be un-related, I am excited to see this finally make it in.
Comment #49
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedSo the entity translation processor issue looks like known issue: https://www.drupal.org/node/2777217.
Comment #50
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedI found one issue and I am not sure what the solution might be or if its a facets issue or not but it looks like the field stored for hierarchical fields are coming back a node entities and not taxonomy entity types. which is causing a few issues displaying the label for the taxonomy ID.
Comment #51
borisson_That's a good question, I don't think we've tested those together already but will defer to @StryKaizer, he's been more involved in this work.
Comment #52
StryKaizer#50 not sure what you mean.
How did you index your taxonomy terms? It should be indexed using tid and adding a hierarchy processor in search_api
My terms are getting the correct labels using the facet processor which converts entity ids to labels.
Comment #53
aspilicious CreditAttribution: aspilicious commentedI get correct labels too.
Comment #54
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commented@StryKaizer and @borrisson_ I indexed the top level taxonomy term, which happens to be the tid which works with the current implementation of the entity processor. What I did find was that if you drill down in the taxonomy term and index the actual term ID
field_name:entity:tid
the processor will through the error identified in #49. I believe what I was originally seeing was a caching issue with the processor not working with Hierarchical facets but I do think there is still an issue with the null call on getSettings() in the Entity processor. I added a patch in the other case worked for my particular case.Comment #55
smiletrl CreditAttribution: smiletrl commentedI can confirm #39 patch works for my case. The hierarchy can be retrieved perfectly.
The translate entity label issue is kind of not related with hierarchy issue. So it would be great #39 patch can be committed.
Comment #56
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedI been testing the patch out and I am seeing the same error as described in #42 but I do not have the patch for #2735891 applied. Oddly enough, everything seems to function without issue, the facets is applied to the URL, if the children aren't expanded they expand correctly. Clicking on the top most parent is the only way to cause the issue; if you select a child even is that child has children of its own no error. I am going to see if I can make sense of where its happening.
stack trace:
Comment #57
borisson_Comment #58
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedSo I have been working through a bit of this here tonight and I think I found one issue that was causing a potential configuration issue
I think the
'#default_value' =>
function call is incorrect. I think instead ofgetExpandHierarchy()
it should begetEnableParentWhenChildGetsDisabled
I am also seeing that the processor essentially not working. The parents are still being selected even with the above code change.
Comment #59
spydimarri CreditAttribution: spydimarri commentedI have been testing Hierarchical Facets. I understand that when we deselect a child the parent is automatically selected. I am seeing a weird behavior if a parent has two children. I selected the both the children I see some search results narrowing it down according to my selection but when I uncheck the one of the children the parent gets selected and this is actually showing wrong search result. As a front-end tester this travelling back up the tree is very confusing for a user. It would be nice if we unselect the children the parent gets unselected as well
Please look at the screen shots uploaded in the file sections
Comment #60
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedI was able to solve the disabling parent term functionality that isn't working with patch #39. The url was not being set correctly and was using the URL from the parent even with the the Enable parent when child gets disabled disabled. The interdiff is against #39 and includes the fix described in #58
Comment #61
StryKaizerThanks, looks indeed like a c/p error.
Are you sure this changes anything (apart from unneccessary code running multiple times)?
It looks like this only triggers the createFromRequest and setOption multiple times, but the output should be the same...
Comment #62
StryKaizer#59There is a setting which makes it behave as you expect, just uncheck "Enable parent when child gets disabled"
Comment #63
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commented@stryKaizer what I was seeing was when the
$url
was being set outside of the$results
loop there was a remnant of the previous terms query param being added to the checked facet here:The if statement in the above will evaluate to false for the checked facet but the
$result->setUrl($url)
, since the$url
was just set by the last loop, will set the selected facet'sdata-facetredir
to have a redirect to that term in the last loop.Resetting the
$url
for each$result
makes sure that the$url
isn't being reused.I think we could also just move the
$result->setUrl($url)
inside the if.Comment #64
mpp CreditAttribution: mpp as a volunteer and at AmeXio commented#56 is right the notice occurs without the current search patch.
Comment #65
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedComment #66
jefuri CreditAttribution: jefuri as a volunteer commentedI applied the patch to drupal 8.2.3 and found that some of the new plugins were placed in the wrong directories. I fixed it and created a new patch from those changes.
One of the issues I saw was that in my drupal root directory a src folder was created with the Hierarchy folder containing the HierarchyInterface, HierarchyPluginBase, HierarchyPluginmanager files.
Comment #68
borisson_@jbertoen The patch is to be applied from the facets directory (
cd modules/contrib/facets; curl https://www.drupal.org/files/issues/implement_hierarchical-2807333-65.patch | git apply
- not the core root folder.Comment #69
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedFacet sorting still seems to go wrong when you enable "Sort by active state" (Sorts the widget results by active state).
Comment #70
dermarioI will try to write some tests for it.
Comment #71
dermarioHere are my tests for this issue. It took me a while to generate all the structures but i think i got it working. Lets see what the testbot says. If there are any cases missing, let me know. During my tests i ran into a configuration schema exceptions, due to a missing schema for the translate_entity option. Thats why i also edited facets.facet.schema.yml although this is obviously not in scope of this issue. Maybe there are more schema entries missing?
In my tests i do the following stuff:
setup:
tests:
Comment #72
dermarioComment #73
borisson_I just did a review and I have no remarks. It looks great. I'll have another look tomorrow but that's absolutely great work @dermario!
Comment #74
aspilicious CreditAttribution: aspilicious commentedCertainly good enough for now.
I'm using this in production for a while now, so I would love to get this in.
Comment #75
borisson_Looks like a couple of things slipped in with @dermario's patch that shouldn't have. Applied #65 + interdiff from #71 locally. Result is attached. If testbot's green I'll commit this right away.
Comment #76
borisson_Awesome, thanks so much everyone. Please open followups if the behavior isn't as expected yet.
Comment #78
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commentedSweet. One step closer to a beta!
Comment #80
PeterStub CreditAttribution: PeterStub commentedI'm using search api(backend = database) - and I see the hierarchy checkbox. I have a commerce product with term references - and I see the facets - but the hierarchy is flat. I dont know exactly what is meant by "Requires the hierarchy processor configured in search api for this field" - I havnt be able to find how to disable/enable this. I guess thats where my problem is.
Can someone point me in the right direction?
I'm sorry if this is not the right place?
Tks
Comment #81
ndrake86 CreditAttribution: ndrake86 at Workday, Inc. commented@peterStub You need to enable the search API processor the Hierarchy, which is located here
/admin/config/search/search-api/index/<index_name>/processors
Looks something like the attached SC.
Comment #82
PeterStub CreditAttribution: PeterStub commentedTks @ndrake86...I did visit that page once - but could see it - but its there now...GREAT - tks alot...
And I confirm it all works...
Comment #83
StryKaizerWe could at the "/admin/config/search/search-api/index//processors" url in the description too in a follow up to help future users.
Comment #84
borisson_#2833681: Improve hierarchy description opened.