Reflect hierarchical taxonomy vocabulary in facet

Very Nice!!!

Attached screenshot comparing before/after. SO much better =)

Two things:
* Could you please submit a patch against DRUPAL-6--1 (dev)
* It seems somehow the character you tried to use inside theme_apachesolr_facet_item_indent() didn't get through (My editor in UTF mode shows it as just "?"). Could you try another character or sending the patch as UTF?

function theme_apachesolr_facet_item_indent($indent = 0) {
  if ($indent > 0) {
    return str_repeat('  ', $indent) .' » '; // I replaced the ? char with »
  }
}

Confirm patch works. However, it added/removed some blank lines; re-rolled against latest DEV.

Also, maybe could we do better than >? =)

Just noticed that this needs to be attacked in the "Current search" block too, as hierarchy isn't being reflected.

E.g. after drilling down thru "Fruit" > "Apples" > "Red delicious", the current search block doesn't show this hierarchy, and then you can remove parent facets (e.g. "Fruit") and end up with children facets active (e.g. "Red Delicious"). This also causes the new hierarchic view to go nuts also. See attached screenshot.

There is a bug in this where if you delete a taxonomy term after the tid is indexed it bombs out the screen.

Also the $vid is hardcoded at substring pos of 7. Mine needs to be 12 for whatever reason (older version?), is it possible to not rely on substring?

+1 for this to make it in the next apachesolr release
I ran into the small sorting bug mkalkbrenner was talking about myself (http://drupal.org/node/412332); one more reason for it to make it in the next rel

See attached example. The metadata conditions can be easily explored by opening and closing the subtrees without selecting them.

@ mkalkbrenner:

Suggesting for the patch to work as shown in screenshot.

Hi, could you tell me if this patch is likely to work against the 6.11 release?
If so, can you confirm that it was written relative to the Modules directory NOT the root directory?
Many thanks

@mkalkbrenner
Hi I am running Beta 5 so applied your 'clone' patch for this version which seemed to change the file as required (though I received no 'successful' or 'failed' messages-but the code looked like it had changed as expected). However, when I checked that my Apache search was still working I received the following WSOD message.

Fatal error: Class 'Solr_Base_Query' not found in C:\wamp\www\drupal2\sites\all\Modules\apachesolr\apachesolr.module on line 856

Now, I also tried to apply a patch yesterday to ApacheSolr which also appeared to be successful but resulted in a different WSOD so I may be doing something wrong. Let me tell you what I did and maybe you can spot the error.

I uploaded the patch file into Wordpad and saved as a text doc (as I am using Windows)
I placed the patch file inside the apachesolr file, navigated to the apachesolr file and typed "patch -p0 < Solr_Base_Query.php_.clone_.patch" in Cygwin.

Reversing the patch ("patch -p0 -R < Solr_Base_Query.php_.clone_.patch") does not get rid of the problem. I have to replace the Solr_Base_Query.php file completely before the problem goes away.

Sorry, wasnt sure if this should be posted on the thread which started with the patch in question (but decided to post it here as my primary objective is to apply the 'hierarchical' patch) apologies if it should.

according to above comments, patch doesn't apply.

Also, looks to me like there is too much being hard coded here - does this really need to be in the main module, or can an add-on module achieve this output?

It works. But...

I'm thinking we should really use theme_item_list() specifically defining children of other items... e.g.:

theme('item_list', array('data' => 'fruit', 'children' => array('apples', 'oranges')));

This should also then work with date facets, for instance.

However, this is IMO, I think the patch could go on as is for now until we get a more elegant solution? Opinions?

as above, this needs to be done in a more generic way versus hard-coding it to taxonomy. I'd rather see more serious if necessary to support such a feature.

Here's a patch that builds an array of children term elements to go into theme_item_list; this does away with the "indent" functions.

Attached screenshot of how it looks.

BTW looked around and couldn't find a better solution than using eval() to build the array programatically (which feels like a hack); suggestions welcome.

Here's a patch (complete I hope) to do some clean-up of the facet code/theme functions: http://drupal.org/node/463900#comment-1593394

That might help simplfy the changes needed here. If not, please suggest additional improvements.

@janusman - why is the eval() needed at all?

@pwolanin: the eval() helps build the nested array of items to later pass on to theme_item_list(). It's a fast way to add items to existing portions of the array at any depth; I can't find (yet) how PHP could handle passing pointers to array elements--I don't think it can.

I just thought that eval() might not be secure in this case if a facet value includes an apostrophe. So that would have to be fixed.

Is it ok to keep this approach (just adding the above security check) and just reissue a patch when #463900: facet theme function clean-up is done? Or, what's missing?

@janusman - you can certainly have references to array elements, and maybe I'm misunderstanding what you are trying to do. For a use of array elemnt references in core see: http://api.drupal.org/api/function/menu_tree_collect_node_links/6

I am realizing that, yes, *I am not sneaky enough* =)

Will write or adapt a recursive function to do the job without eval().

Good starting point:
http://kevin.vanzonneveld.net/techblog/article/convert_anything_to_tree_...

Ok, new patch, does not use eval().

@mkalkbrenner: I'd say this is RTBC... have to take this one patch at a time =)

looks liek there are some code style issues in the changed code in terms of whitespace.

Also, I'm not sure I understand why we need the static cache and what this function is really doing:

  function apachesolr_get_unclick_link(&$query, $field_name, $field_value, $vid = FALSE) {

I'ts taxonomy specific?

I feel like I am missing something here,
can you explain and maybe illustrate the
desired final behavior.

I still would like a clear summary of the goal - which links are supposed to appear when, how are unclick links supposed to work, etc. I can't really review the patch without that.

Great feature. Subcribing.

I don't want to derail this very nice feature but I do want people to join me in thinking about ways to make facet block display pluggable (as well as the logic behind it all). for example, I've come up with two alternate ways to display facets (tag clouds and bar graphs) that will sit nicely alongside this hierarchical display and the normal facets. It would be nice if we could generalize how the admin chooses displays and how the system handles them. Investigating this patch might be a source for inspiration to this end.

Nice work so far here. My read of the patch didn't uncover anything obvious, so I need to test it thoroughly before commenting further.

I'm am unhappy with the use of this function for all facets:

+function apachesolr_search_get_unclick_link(&$query, $facet_text, $field_name, $field_value, $vid = FALSE) {

It seems to add unneeded overhead and complexity for the common case.

As Robert suggests above, we possibly need a way to add information for each facet block about a display handler. So, Basically the functions for display hierarchical vocab facets would substitute for the current ones.

This is starting to sounds a bit like a views block display handler, at least in concept.

For the short term, possibly we could add a link next to each enabled facet to take you to chooser?

@mkalkbrenner: great you're looking into this, will test it soon!

Now, re: #44, #45...

Thinking in this more general framework, IMO this patch is about improving the current "facet block display handler" which turns out to be the one for "taxonomy".

I agree 100% with trying to build a flexible framework for facets (display handlers, hooks, etc); and yes, it starts to sound a bit like views =) In fact at DrupalCon DC I suggested to @afummy that maybe, ApacheSolr could just be the backend for Views, and Views UI could behave like faceted search (where all options shown to "filter down" indeed produce >0 search results)... just an idea =)

However, IMO @robertDouglass's #44 and @pwolanin's (and my) agreement on it is farther down the road; if not we will get this module to final 1.0 in another 12 months =)

Can I convince you to move this issue ahead and open a new one to look into more into the facet framework (?) for Apache Solr?

Like I said, I wasn't trying to derail this feature.

@janusman - apachesolr_views module makes Solr the backend for views. It works, pretty much like you describe.

I'm want to contribute testing for the patch in comment #42. But the patch didn't work. Is it normal?

First I apply unclick.patch. Terminal returns

patch: **** Only garbage was found in the patch input.

Then I apply apachesolr.hierarchical_facet_10.patch. Terminal returns

patching file apachesolr.module
Hunk #3 succeeded at 1405 (offset -1 lines).
patching file apachesolr_search.module

Then when trying a Solr search at mywebsite.com/search/apachesolr_search/ Drupal returns the below error. It was working fine before the patch.

Fatal error: Call to undefined function apachesolr_search_localize_taxonomy_term() in /home/mydomainname/public_html/sites/all/modules/apachesolr/apachesolr_search.module on line 602

Things I tried: Rebuilding Solr index, run cron.php then wait 5 minutes before testing again.

I'm using Ubuntu Server 8.04.2 via SSH. Patches applied against current head apachesolr-6.x-1.x-dev.tar.gz 2009-Jun-19.

I'm assuming that this patch indent the children facets. It's a great feature. It improves usability.

I suggest another improvement to the current patch. Adding an unlimited facet depth instead of the current limited depth. Find attached screenshot to clarify. The unlimited depth is use with the Faceted Search module. You can try it at: http://facetedsearch.davidlesieur.com
This demo uses unlimited facet depth so you'll still see only top level facet in the first step. If you click on it, the search result gets filtered and the child facet of that top level term having results will appear. Clicking the 'all' link brings you back to the top level facet. Notice the use of two colours: Grey for not clickable items such as arrows and (number) and red for the facets. Using two colours make the block easier to read and looks better.

The issue with patch #42 is that if the Solr block is 200 pixels width fixed. The depth is limited to 3 or 4 facets maximum. With 5 facets depth or more the fixed block might not be wide enough and the facets will start distorting the block and the website design/layout.

With suggested unlimited facet depth if the Solr block is 200 pixels width fixed the depth is unlimited. When the facet reach the right side of the block it simply continue on the next line and so on. Unlimited. Works for both fixed and expendable/fluid design. Find attached mockup to clarify. This is how the suggestion works.

I'll try to review in depth today. I see some obvious problems though, mostly style/readability. Like:

$link = apachesolr_search_get_unclick_link($query, $term->name, 'tid', $tid, $reflect_hierarchy['apachesolr_search'][$delta] ? $vid : FALSE);

apachesolr_search_get_unclick_link is only really operative for hierarchical taxonomy, right? The use of the ternary operator in the funciton call is hard to follow, and really it doesn't make sense to me to ever call this new function unless we are representing a hierarchy. So, I'd rather see code more like:

if ($reflect_hierarchy['apachesolr_search'][$delta]) {
  $link = apachesolr_search_get_hierarchical_unclick_link($query, $term->name, $tid, $vid);
}
else {
  $link = theme('apachesolr_unclick_link', $term->name, $new_query->get_path(), $options);
}

The current special case of $vid and 'tid' could then just be the code that always runs in apachesolr_search_get_hierarchical_unclick_link().

This second option of a more generic function looks more useful/re-useable to me.

In terms of performance, I doubt this 2x cloning has measureable effect on page load, and also this might lead to problem if one wants to do for example, a federated search (e.g. separate user and ndoe searches shown on the same page). In the absence of compelling evidence that this optimization is needed, I think it should be omitted.

In terms of actual performance, I'd bed the Frankenstein use of page_query() is more of an issue, since I'd think this issues a couple actual SQL queries which do essentially nothing.

Why not just do:

$remove[] = $field;

instead of:

$remove = array(array($field['#name'], $field['#value']));

Then you can reference the named keys instead of 0, 1

I still think we should be using named keys vs. 0, 1

Patch applies cleanly to fresh checkout.

It works, however, if the user is directly limiting by a deep-levelled term (which will happen if "Use Apache Solr for taxonomy links" in admin/settings/apachesolr/settings is set), then the "Filter by" facet block fails to show the lineage (it just shows the top-level parent as available).

See attached image: on the left is the result of normal search + filtering; on the right is what would happen if I would click on the term "Economics" from inside a node's taxonomy term links, or if a user somehow has a direct link to that term.

mkalkbrenner this issues has been slated to still go into the 6.1 branch - you won't be wasting your time.

@mkalkbrenner: need help? =) Contact me or leave a note here.

@mkalkbrenner: Those pesky projects, eh? =) Good luck on that.

I prefer (1). I think the code should always try to build the lineage every term if it hasn't already.

E.g. Fruit (tid:1) > Apples(tid:2) > Granny smith(tid:3)

Case 1) filters=tid:3
Build the lineage for all parent terms of tid:3

Case 2) filters=tid:1 tid:3
Build lineage for tid:1
Build lineage for tid:3 (add tid:2 as son of tid:1, add tid:3 as son of tid:3)

Case 3) filters=tid:1 tid:2 tid:3
Build lineage for tid:1
Build lineage for tid:2 (add tid:2 as son of tid:1)
Build lineage for tid:3 (add tid:3 as son of tid:3)

I'm thinking this might be a tad expensive query-wise if we don't do it correctly. Perhaps we might even come up with a novel solution to do it all just using Solr =)

I think I have some time thursday + friday for this, Ill look into it.

A paper describing Castanet, an algorithm for automatically generating hierarchical faceted metadata.

Automating Creation of Hierarchical Faceted Metadata Structures
http://biotext.berkeley.edu/papers/castanet.pdf

I'm backing off my statement that this can still go into 6.1. I want to get that branch locked down without introducing more moving parts. Changing to 6.2.

coupet - the article was quite interesting, but not relevant to this issue.

As to comment #64, I also think approach #1 is correct. A child taxonomy link, at least in the context of http://drupal.org/files/issues/401234_60.png , should include filters for its visible parents.

New patch built upon the previous one by @mkalkbrenner et al...

Overview:

• Adds an option to each Filter by Taxonomy block in admin/build/block/configure/[module]/[delta] to show that vocabulary as hierarchical or as flat.
• Shows thus-configured facets as hierarchical, both in the "Filter by.." block, and in the "Current search" block.
• Allows the keyword search to be removed (taken from #473554: Add an "unclick" link to search facet block)

It also now fixes the problem seen in #60.

Code-wise:

• it moves the "current search" block code from apachesolr_search_block() into its own function for legibility. This is basically the reason it includes the patch from #473554: Add an "unclick" link to search facet block since that other patch
• it adds a new public function to Solr_Base_Query.php : remove_keys().

My feeling is that the code could still be optimized a bit, but it seems stable and handles entry to a deep-nested term correctly.

And... a question:

Is there a use case for admins wanting to show hierarchical facets as flat/unrelated?

My first impulse is that it would be very unlikely...

In fact I think *not* showing terms hierarchically (IMO) can introduce usability hurdles into a site. Because of this, I'm asking here (@robertDouglass??) to reconsider having this "fix" go into 1.x.

Can't get this patch to work. Anyone willing to share a working patched module, please? Kindly contact me directly if you wish to help. Thanks.

I think this issue might allow us to simplify the patch slightly? #545094: Method to set Keys on Query

+  $reflect_hierarchy = variable_get('apachesolr_facet_reflect_hierarchy', array());
+  $form['apachesolr_facet_reflect_hierarchy'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Links reflect hierarchy'),
+    '#description' => t('Facet will be sorted hierarchically if it bases on a hierarchical taxonomy vocabulary.'),
+    '#default_value' => !empty($reflect_hierarchy[$module][$delta]),
+  );

Is there any reason why we don't do this automatically for all hierarchical taxonomy vocabularies? This would certainly be more user friendly, IMO.

Please, no more _functions. "Private" functions need to be class based or they're not private.

+function _apachesolr_search_get_unclick_query_key($res, $val) {
+function _apachesolr_search_nested_facet_items($input, $parent = '', $level = 0) {

I think the doxygen standard that we use is to put the type and variable on one line and the short description on the next line.

 /**
+ * Creates a link to trigger a new solr search after removing at least one of previously
+ * selected search filters. Therefor you have to pass the filters' field names and values.
+ *
+ * @param $query object current apachesolr query
+ * @param $facet_text string
+ * @param $remove array array(array('#name' => 'field name', '#value' => 'field value'), array('#name' => 'field name', '#value' => 'field value'))
+ * @return string themed unclick link
+ */

Why is $query passed by reference?

+function apachesolr_search_get_unclick_link(&$query, $facet_text, $remove) {

Coding style $a . $b

+function _apachesolr_search_get_unclick_query_key($res, $val) {
+  return $res.$val['#name'].$val['#value'];
+}

Full doxygen comments please:

 /**
+ * Return nested array of nested facet values for use with theme_item_list()
+ */
+function _apachesolr_search_nested_facet_items($input, $parent = '', $level = 0) {

Also, it can read: "Return nested array of facet values" (redundant "nested")

Either give a type hint in the function signature or cast $input to an array so we never get warnings on the foreach.

 /**
+ * Return nested array of nested facet values for use with theme_item_list()
+ */
+function _apachesolr_search_nested_facet_items($input, $parent = '', $level = 0) {
+  if ($level == 0) {
+    ksort($input);
+  }
+  $result = array();
+  foreach ($input as $key => $item) {

Full doxygen comments please:

+/**
+ * Return the contents of the "Current search" block.
+ */
+function apachesolr_search_currentsearch_block($response, $query) {

Extra commented line?

+  #$links[] = apachesolr_l($query->get_query_basic(), $query->get_path(), array('attributes' => array('class' => 'active')));

Implemented changes @robertDouglass mentioned in #72.

Patch doesn't apply to DRUPAL-6--2?

patching file apachesolr_search.module
Hunk #2 FAILED at 653.
Hunk #3 succeeded at 784 (offset 5 lines).
Hunk #4 succeeded at 834 (offset 5 lines).
Hunk #5 succeeded at 1203 (offset 5 lines).

!

Maybe 2.x changed before this patch =) Rerolled.

Forgot to mention that now there's no manual setting needed, the "show hierarchy" setting automatically applies to all vocabularies that are not freetagging nor multi-parent hierarchical (as determined by taxonomy_check_vocabulary_hierarchy()).

Has anybody tried to use this in combination with content taxonomy fields? I am currently having a hard time trying to implement a solution. :-/

what exactly do you mean "content taxonomy fields"?

let me explain: by using the http://drupal.org/project/content_taxonomy content taxonomy module, it is possible to create cck fields using a taxonomy. the storage for those content taxonomy fields differs from the "normal" vocabularies if the setting "add to core taxonomy" is disabled (which is the case for many content taxonomy fields I am using for a project).

It is currently not possible to use those cck fields as a hierarchical facet since their string doesnt match to strpos($delta, 'im_vid_') === 0.

An apparently correct but at second thought wrong solution could be altering a document prior uploading it to solr by parsing all given cck fields, checking if they are from the type "taxonomy" and set the values in analogy to the core taxonomy fields (eg similar to apachesolr_add_taxonomy_to_document) BUT: That would eliminate the information about our cck field! Imagine two content taxonomy fields using the same vocabulary. By adding all content taxonomy fields to the tid, im_vid_# and vid values it would be still not possible to filter for a specific cck field, but only for a specific vocabulary.

The result is that content taxonomy fields need their own sets of multivalues, eg im_tid_MYCCKFIELD and im_vid_MYCCKFIELD_vid_# etc. But this means that almost the entire code has be adapted. :-/ After several hours of work I got some results but I am not really convinced by it; I used hooks whenever possible or added some new hooks and rebuilt a big part of this patch in my own search module (instead of directly adding things to the module) since patching would become even more terrible if I would directly modify the code :-(

Any ideas? Is there somebody else who ran into the same problem?

ruxkor: One solution might be to write a module that provides facet blocks for content_taxonomy. The core apachesolr modules are unlikely to support this directly, and as you pointed out, the important bits are rather straightforward string comparisons. Please open a different issue as it's not actually directly related to this one. Thanks.

@ruxkor: I am currently using content_taxonomy fields in our production site; we *do* have "Additionally store in the core taxonomy system" option checked for all these fields so Apache Solr works out of the box with those fields.

If you want to go this way, then try looking at this issue: #485328: Provide a way to import and migrate from traditional taxonomy field ... we had to re-import our field values into the CCK tables during our migration to D6.

Patch didn't apply anymore, rolled a new one.

Patch failed to apply, bad line in Hunk #2... new patch.

We should probably get this in 1.x first if we want to reach 1.0?

This is frustrating; since the patch touches a lot of code and the module itself (the 2.x branch anyways) has been changing frequently, it has to be reviewed quickly or else it won't apply when a reviewer comes along. =)

Perhaps 6.x-1.x is a slower-moving target?

If I can get confirmation from someone willing to review this, I can re-roll the patch (just tell me which branch I should aim for!) =)

I've been trying to find time to review - if you can re-roll for the 1.x branch then ping me about it.

We all agreed this would go in the 1.x branch, so let's try to get the code right there and then port it.

New patch, this is for 6.x-1.x-dev.

looks like there are unrelated changes in the patch to Solr_Base_Query.php

Correct. New patch.

Looks like this ought to be using constants defined by taxonomy module?

if ($voc->hierarchy != 2 && $voc->tags != 1) {

Uhm... there are no defined constants in the D6 core taxonomy module... same in D7 =)

To prove it, here's a bit of code inside taxonomy_help()... the rest of the code also compares tags and hierarchy to ">0", "==2", etc.:

    case 'admin/content/taxonomy/%':
      $vocabulary = taxonomy_vocabulary_load($arg[3]);
      if ($vocabulary->tags) {
        return '<p>'. t('%capital_name is a free-tagging vocabulary. To change the name or description of a term, click the <em>edit</em> link next to the term.', array('%capital_name' => drupal_ucfirst($vocabulary->name))) .'</p>';
      }
      switch ($vocabulary->hierarchy) {
        case 0:
          return '<p>'. t('%capital_name is a flat vocabulary. You may organize the terms in the %name vocabulary by using the handles on the left side of the table. To change the name or description of a term, click the <em>edit</em> link next to the term.', array('%capital_name' => drupal_ucfirst($vocabulary->name), '%name' => $vocabulary->name)) .'</p>';
        case 1:
          return '<p>'. t('%capital_name is a single hierarchy vocabulary. You may organize the terms in the %name vocabulary by using the handles on the left side of the table. To change the name or description of a term, click the <em>edit</em> link next to the term.', array('%capital_name' => drupal_ucfirst($vocabulary->name), '%name' => $vocabulary->name)) .'</p>';
        case 2:
          return '<p>'. t('%capital_name is a multiple hierarchy vocabulary. To change the name or description of a term, click the <em>edit</em> link next to the term. Drag and drop of multiple hierarchies is not supported, but you can re-enable drag and drop support by editing each term to include only a single parent.', array('%capital_name' => drupal_ucfirst($vocabulary->name))) .'</p>';
      }

Wow, taxonomy module sucks...

Hi,

Suscribe for the hierachy of taxonomy in apachesolr.
Sorry but the hierarchy doesn't work.
I've always a flat taxonomy.
Drupal 6.14. I've apply the apachesolr-401234-91.patch.
Tested with apachesolr 6.1.x DEV and the the 6.x-1.0-rc3.

Really don't understand.

Trying out the last patch - I see the same issue as fabdel - the hierarchy doesn't show.

oh, wait - this is sort of working - but very counter-intuitive to me. The initial search result only shows the top level parent that's matched - no necessarily the actual term in the search results.

Patch needs work no matter what, since it will WSOD if taxonomy module is disabled:

 Fatal error: Call to undefined function taxonomy_get_vocabularies()

Wow.. this is comment #100...

IMO, we *want* to actually display just the top level terms, since there potentially could be too many child terms, they might never be shown. Also, it's in the spirit of "drilling down" to find what you need (which is what faceted search is good for... making large, complex data sets) =)

Say you have terms for the diferent animal kingdoms/Pylums/classes/etc inside a taxonomy. If you do a text search for "wings" it would get messy to display ALL the matching species's terms in an apachesolr block. What you want is to narrow down, starting from the top Phylum then drill down .. until you get to the insects or birds, etc.

This is the way a lot of faceted search interfaces work. If you're unconvinced I can look up a few cases.

I'll fix the patch to check for the taxonomy dependency found in #99.

New patch, fixing the comment from #99 if taxonomy module was not enabled.

Forgot to set as needs review.

regarding displaying the hierarchy - we currently display all the parents of any term on a node, so the number problem already exists.

Perhaps what's bothering me is the UI - in this case we kind of need something like the menu block have to indicate that a term has children.

Also, is it the right behavior to always reflec tthe hierarchy, or are there use cases where search results shoudl display a hierarchy flat as we have now?

Nice implementation - I like the check to X transition when you hover over the unclick link.

Agree, nice hover effect =)

With regards to the handling of the block behavior, I'm still in favor of not making it configurable. I will cede the point if others feel strongly about it. There are already many ways (especially in 6.2) to configure blocks, with more on the way, and it is quickly becoming a nightmare to manage that form. That's my motivation for trying to make the apachesolr_search module make as many assumptions as we comfortably can.

Ok. The switch is a non-issue. We leave it out.

And, just because I'm busy right now and can't double check, please explain again how this degrades when no JS is enabled? Thanks.

I don't think we need a switch in the UI - it can be an opaque variable. I just would like to leave the options open.

opaque variable ++
Those never hurt anything.

I still think we should not introduce this to 6.1 at this point in the game. Can we re-roll for 6.2? /me wants to see a 1.0 release for 6.1.

This has been on the roadmap for 6.x-1.0 for a while as the last major feature - just lack of time has prevented us from getting it in.

I'd been intending to find a couple hours and then get this in for 1.x.

@mkalkbrenner's post is an amplified mirror of my opinions =)

@pwolanin please if you need any help, clarification, b33r, etc. PM me.

I'm working on rerolling it, but some serious problems with the last patch including apachesolr_search_currentsearch_block() is never called.

Also: Fatal error: Call to undefined method Solr_Base_Query::set_keys()

The last patch doesn't apply because it conflicts with Robert's last (little) commit.

Here's a patch that applies at least, and no fatal errors.

Hmm, I am still dissatisfied that their is no prior indication which terms have child terms.

Slightly improved patch.

It seems like we could make the parent lookup much more efficient - use a single query instead of a query per term.

@mkalkbrenner - I understand your frustration with a long-winded patch. I'm also not completely happy with adding new features to 1.0 when we should be trying to get a release out, but I find people's dedication to contributing more important, so if the interested parties here dearly want it in 1.0 then I won't stand in the way. It will need to get ported to 6.2 when finished.

Tested, works.

@pwolanin: re 120: Uhm, I don't think we're doing anything similar with the date facets, where it is also not clear that there is the option to further drill down by day, hour, etc. So I don't think the visual aspect for this is a blocking issue, but agreed, it's something that can be improved on across the whole module. Maybe we should think of rendering menu-like facet trees using menu_tree_output() using a faux-pas tree? Or just use core's classes for menu items... I suggest we open a new issue for this.

re 121: I will do some benchmarks to see which is better performance-wise. I don't think it's a blocking issue either.

I'm not happy yet with the architecture of the code, even though it works.

There is some kind of funny string parsing going on to keep track of the depth of each item.

Hmm, it's funny that you set the patch to needing review when you already knew the outcome... maybe you didn't mean to?

The string parsing going on is just a way to define (via the key) the number of terms, the term id, and the parent-child relationship in a way that can IMO be parsed simply (with little code) and quickly. Maybe you want a data structure of nested arrays or objects?

Please indicate if you need comments sprinkled/doused on the code... or any other specific help on this (please be specific). It's not worth putting any more time into this if it'll still get bounced due to Mysterious Reason N+1 =)

Removing myself from "Assigned:"

I'm only putting to CNW because I was planning to work on the code a little more myself.

Apologies, I assumed the CNW was meant for me because I was still assigned to the issue. =)

no, sorry - I generally don't pay much attention to the assigned flag once I start looking at a patch :-)

This isn't working yet, but partially re-written.

Could substantially reduce the # of queries, but is possibly a little less robust to there being stale data in the index.

The code in the current search block potentially generates the same unclick link multiple times. I think this line in the new function apachesolr_search_currentsearch_block() will be called 2x for the same key if a parent and child term are both in the current search block:

$active_facets_unclick[$facet_fieldname][$key] = apachesolr_search_get_unclick_link($query, $fielddisplay, $remove);

Here is a massively revised patch.

Please review to see if you follow the logic of the code - it uses variable references in a way that's hopefully transparent enough to understand.

The code added here will can be used to render any properly constructed hierarchical facets.

There is a slight amount of work to be done yet in terms of perfecting the classes for items that have children but are collapsed, versus having children and open.

some minor cleanup.

This is going to be fun to forward port :\

The thing the jumps out at me as being ugly is how we get facet texts. Horrible. But that's a separate issue.

I'll test in the coming days. Looking for feedback from janusman and mkalkbrenner as well.

oops - that last patch missed the minor change to the query class.

Hopefully should not be too hard to port - basically jsut moves all the currentsearch and taxononmy facet code out of the big hook_block into separate functions,

Haven't tested it yet, just reviewing the code...

+++ apachesolr_search.module	29 Nov 2009 14:01:30 -0000
@@ -690,6 +624,90 @@ function apachesolr_search_block($op = '
+          // We don't display children unless the parent is clicked.
+          if (!$query->has_filter('tid', $term->parent)) {
+            unset($facets[$term->tid]);
+          }
+          else {
+            $facets[$term->tid]['#parent'] = $term->parent;
+            // Use a reference so we see the updated data.
+            $facets[$term->parent]['#children'][] = &$facets[$term->tid];
+          }

I think the above could cause a problem if the user enters a search from the taxonomy pages override (e.g. going to /taxonomy/term/xxx). Will test...

And, in apachesolr_search_currentsearch_block()... am I misreading, or are we only handling keys and taxonomy terms in the current search block? (No dates, authors, etc?)

The current search block special-cases keys and taxonomy, but all others should still work.

Ok here's what I've found:

1) My observation from #135 is true: I have this taxonomy:

• Engineering & Applied Science (taxonomy/term/62547)
  • Applied Physics (taxonomy/term/62712)

If I have "Use ApacheSolr as the default for taxonomy term pages", and I go to an inner term (say, "Applied Physics" from above), then the current search and the facet block for that taxonomy are confusing:

• Current search block: only shows "Applied Physics", as an active filter.
• Hierarchical facet term block: only shows the parent "Engineering & Applied Science", inactive.

2) Something definitely broken with the nesting:

3) The expand/contract class handling is awesome! =) Just that you're adding a class called "expanded-facet", maybe you meant "expanded"? If you did mean to add in a new class, would there be any harm in also adding the "expanded" class?

A possible clue to #137 (2):

I think somehow the algorithm is missing every even-level item. In the above screenshot it missed showing and "Business" (2nd level), "Finance (General)" (4th level).

*EDIT* I meant they are just "missing" in the rendered facet block; they are indeed indexed properly.

This patch fixes issue 2 from #137.

Another issue: the "collapsed" class on list items depends on the returned facet values but should instead depend on the database. This is because the Solr facet list could get cut off just enough in some cases to not show properly:

The attached patch fixes this issue, here is a diff between the patch in #139 and this one.

Given that we expect relatively few (~20 ?) items per block response, can't we convert the query to an OR query rather than doing the join back as you have?

i.e. something like:

 $result = db_query("SELECT tid, parent FROM {term_hierarchy} WHERE tid IN (". db_placeholders($facets) .") OR parent IN (". db_placeholders($facets) .")", array_keys($facets), array_keys($facets));

I'd side with the best-performing query =)

ok, well I don't have a good data set to bench mark, but we might (or might not) make the code faster by also adding the condition parent > 0, which is really what we want.

Here's a patch - can you try?

It's also possible that 2 queries will be faster?

Your query performs better.

You only missed a spot:

+++ apachesolr_search.module	1 Dec 2009 01:33:37 -0000
@@ -690,6 +624,88 @@ function apachesolr_search_block($op = '
+      $result = db_query("SELECT tid, parent FROM {term_hierarchy} WHERE parent > 0 AND (tid IN ($placeholders) OR parent IN ($placeholders))", $tids, $tids);

"$tids, $tids" should become "array_merge($tids, $tids)" :

+++ apachesolr_search.module	1 Dec 2009 01:33:37 -0000
@@ -690,6 +624,88 @@ function apachesolr_search_block($op = '
+      $result = db_query("SELECT tid, parent FROM {term_hierarchy} WHERE parent > 0 AND (tid IN ($placeholders) OR parent IN ($placeholders))", array_merge($tids, $tids));

So this fixes issue (2) from #137.

The only thing left I see is issue (1) from #137: starting out in a child term search, from either a taxonom/term/XXX page override, or search/apachesolr_search/?filters=tid:XXXX, shows confusing links. (See the description and screenshot in #137)

oh right - here's a fixed patch with the array_merge.

The only thing left I see is issue (1) from #137: starting out in a child term search, from either a taxonom/term/XXX page override, or search/apachesolr_search/?filters=tid:XXXX, shows confusing links. (See the description and screenshot in #137)

I see it as a problem; anyone have an opinion on it? =)

This seems like a separate bug - why is this tid not in the list of active filters in the query object?

Ah, maybe this is a bug - the previous code was finding all parents. Too. The facet that the facet block is not correct confuses me.

Hmm, there are possibly sql rewite issues we need to ponder as well.

The problem from #146 is this:

Going directly to a "deep" term for an URL like search/apachesolr_search?filters=tid:2 or taxonomy/term/2 causes the problem because the parent (or parent's parent, or...) is NOT in the URL and the code just getting the inmediate parent (not the parent's parent, etc).

Before we embark on any fix, my question is: is this a problem related to this issue, or a different issue altogether?

I'm thinking this is a different issue, introduced by #472600: Replace taxonomy/term/XXX node listings with Apache Solr search results. Thoughts?

Maybe some way to strip this code down further? Feels like a lot of bloat to load on every page.

I think it at least addresses the bug raised in the last screen shot.

Slight tweak

Awesome.

Tested, fixes the problems I mentioned... I think this is RTBC.

I think I may still have sen some bug where the child of an active link was not shown, but in general it seems to work right.

Let's get this in - then maybe we can think about some unit or system tests even to look for broken behavior.

committing to 6.x-1.x.

@janusman - do you have a reasonable sized text vocabulary we could use? Or a sampling of it?

re #154: Yes I do have a large hierarchical vocabulary; to get it install this module:

http://drupal.org/project/hilcc

which will create a new taxonomy with some 1,000 terms... when enabling it will show a message pointing to the newly-created vocabulary.

The hierarchy and the check boxes don't work well together. One possible solution is to try to clean it up with CSS. Another possible solution is to not to checkboxes on expandable items. Thoughts?

Committing this patch as a stop gap to stay in sync with 6.1

Hi. I just spent the past hour or two trying to figure out why my taxonomy facet block, which is for a hierarchical multi-parent taxonomy, was not displayed that way. Here's why. I'm using 6.x-2.0-alpha1.

In apachesolr_search_module, inside of the function apachesolr_search_get_hierarchical_vocabularies(), the following statement exists:

      foreach ($vocabularies as $voc) {
        // If the vocabulary is not multiple-parent hierarchical and not
        // freetagging and not designated to be forced to display flat.
        if ($voc->hierarchy != 2 && $voc->tags != 1 && empty($force_flat[$voc->vid])) {
          $result[$voc->vid] = 1;
        }
      }

In my case, my vocabulary is a multi-parent taxonomy and it is hierarchical. The first part of this if statement fails because $voc->hierarchy equals 2. The result never gets set to 1, and my block displays the content in a flat list.

I read post #76 above and this seems to be intentional. Question: why do we disable hierarchy for multi-parent taxonomies? Would this break if I modified that if statement to display them?

Thanks.

Committed to DRUPAL-5--2, in #318058.

Patch attached.

After appying the patch from #160 I notice that the filters text for CCK filters, from Current Search block are disappearing.

This because the facet text is build in apachesolr_search_nested_facet_items() in the line:

    $facet_text = theme('apachesolr_breadcrumb_' . $field['#name'], $field, $field['#exclude']);

But when a CCK field is passed, the theme invoked is not correct... For example, for a CCK field called "field_city" the value $field['#name'] = 'is_cck_field_city' (the Solr mapping) and the invoked theme will be theme('apachesolr_breadcrumb_is_cck_field_city') which does not exists.

would it be possible to have a different facet for each taxonomy level?

The patch has already been ported to 2.x.

The bug in #161 is critical for 5.x-2.x, so that's what this issue will be about. Replying to some other comments:

@Aldus #163: Please create a new issue for discussing having a different facet/facet block for each taxonomy level.

@rjbrown99 #159: Please create a new issue for discussing multi-parent hierarchical taxonomy facets.

@robertDouglass #157: Create a UI issue with a patch.

The bug in #161 was fixed in #724440: Current search does not display properly the CCK filters. RE: #157, #159, #163: create new issues.

This was the last bug in 5.x-2.x. Yay!

its this patch applied on d7 versions?
i can see a Expand hierarchy checkbox option, but it dosen't seem to have any effects(or it has some sort of bug on my websites)

I'd also be interested to know if people are using hierarchical taxonomies for Solr facets in D7.

The Facets are not supplied by this module any more, the are from FacetAPI.
I think this may help you : https://www.drupal.org/project/apachesolr/issues/2954804#comment-12549596