Repeated facets in the Solr Sort block

Interesting. I'm not sure why the sort block was even still enabled on d.o once #425986: Convert project browsing facet blocks to a navigation form was committed and deployed...

But, that said, the sort block shouldn't be broken like this if someone did want it enabled. I'll check with csevb10 about this and hopefully it's an easy fix.

Thanks for the report,
-Derek

Ok, we can definitely attack a bunch of this in different ways, but here is 1 possible solution to the issue.
The problem stems from the need to have all of the sorts available in order to sort appropriately. We add all of the sorts, therefore all the sorts are available, and thus they all show up in the sort block from apachesolr. Luckily for us, apachesolr provides an alter invocation that will let us remove our sorts when they are unnecessary (and/or overkill - such as what you see in the image).

This patch pulls our sorts out so that we can more easily leverage them in the apachesolr_sort_links_alter and separates out the sorts into 2 categories:
1. the sorts that are just project specific
2. the sorts that should always be suppressed.

In the apachesolr_sort_links, we then suppress all of the sorts (in the scenario where we aren't filtering by project), or suppress only those sorts that should always be suppressed for the block display. This allows us to remove the sorts where they're not desired (the apachesolr sort block) while leaving them where they're needed (the query object).

@csevb10: Don't forget to set patches to "needs review" -- that way the test bot will beat on them. ;)

Disclaimer -- I don't know enough about ApacheSolr to know if this is the overall best approach or not. I'd like to get feedback from someone else on that, if possible. I'll try to find pwolanin in IRC. It seems weird to advertise sorts that we then have to undo.

However, assuming this is the best approach, I have some specific concerns with the patch:

A) I don't understand what $suppress_only means, neither as a variable nor parameter name. This PHPDoc doesn't really help (and even if you finished the sentence, I'm not sure I'd understand) ;)

+ * @param bool $suppress_only
+ *   If FALSE, returns all sorts for the project module. If TRUE,

Can we come up with better terminology to represent what this is about so that the code makes more sense, even without PHPDoc?

B) Similarly the terminology in this code:

+    $sorts['display']['ds_project_latest_release'] = array('title' => t('Last release'), 'default' => 'desc');
+    $sorts['display']['ds_project_latest_activity'] = array('title' => t('Recent activity'), 'default' => 'desc');
...
+      $sorts['suppress']['ds_project_latest_release_' . $tid] = array('title' => t('Last release'), 'default' => 'desc');
+      $sorts['suppress']['ds_project_latest_activity_' . $tid] = array('title' => t('Recent activity'), 'default' => 'desc');

doesn't really match what you said in comment #2 about "1. the sorts that are just project specific" vs. "2. the sorts that should always be suppressed."

C) The return values from project_solr_get_sorts() seem really weird. Do you ever get the fully nested array with the 'suppress' and 'display' keys? That needs real PHPdoc at least. I probably wouldn't do the weirdness with array_merge() and all that right before returning. Seems like if you pass it an arg to control which sorts you want, the code should look more like this:

$sorts = array();
if ($something) {
  $sorts['foo'] = 'foo stuff'
  $sorts['bar'] = 'bar stuff'
  ...
}
if ($something_else) {
  $sorts['baz'] = 'baz stuff'
  ...
}
return $sorts;

Then, no one will ever look at the code and wonder if the array they get back is nested or not.

Anyway, I'd like to first understand if it makes sense to go this route at all. If so, we can hash out the implementation details. ;)

Thanks!
-Derek

A) I couldn't come up with something good to call this. Basically $suppress_only indicates that we're pulling a list of only those items that should be suppressed from the apachesolr sort block in all scenarios. In other words, some of the sorts (i.e. those that are version specific -- sort by latest release of drupal 6.x) should never appear in the apachesolr sort block. Others (sort by latest release in general) seem to make reasonable sense to display in the apachesolr_sort block. So we have 2 classes of items: those items that are suppressed all of the time in the apachesolr sort block and those that - depending on circumstance (in this case those items that are project-specific, but generally useful like sort by latest release) are visible. If you can think of a good way to indicate that, I'm open. The naming was not my favorite, but I wanted to get the concept out there for review. (I have no idea how I rest of the comment, but you're probably right in that it wouldn't have helped a great deal).

B) In this case - or with respect to the sort block - "suppressed" indicates those sorts that shouldn't display in the block. They are still usable for sorting, but we don't clutter the block with additional values.

C) The internal naming is just for determining what we're going to use vs what we're not. I tried another implementation previously, and I have a feeling it'd be closer to what you want. That'll be an easy change.

Agreed. I don't like the idea of creating the sorts then simply suppressing them from use in the sort block, but I ran into the following issues (that are worth noting for anyone jumping into the issue):
1. We need all of these sorts in a variety of situations. The prepare_query code is built so that the sorts can be leveraged on the search pages, for instance, whereas the base case is the project pages.
2. We don't have much to case on in prepare_query as in most scenarios we don't have the default filtering of project or module/theme.
3. Moving the sorts later in the process used to break everything. I have made some changes so I may have fixed that issue over time, but at last check, adding the sorts anywhere other than prepare_query - which could lead to us having enough info to properly case whether to or not to present the additional sort criteria - broke the query functionality.
4. Even if we limit the functionality to only relevant locations (aka project page or project-specific solr queries), we still end up with the repeated facets in the apachesolr sort block because the block picks up every available sort.
5. Regardless, the solution always feels sub-optimal because our sorts end up in the available_sorts like where, if another module implements sorting, we still have the possibility of displaying a bunch of unnecessary information.

My next attempt would probably be something like the following:
1. Only enable 1 of the sorts per run for each type (i.e. only enable latest release or latest release for Drupal 6, not both.) This cleans up our sort block.
2. Work on project solr to recognize and shift incompatible sorts (such as latest release for Drupal 6) in situations where they doesn't make sense (aka they haven't faceted on just D6 projects). I am pretty sure this could be done.
3. Move and test adding sorts to modify_query (where all of the filters have been set). This will allow us to determine at the point we add sorts whether the sort should or should not be added. This did not used to work, but I hope that the other clean up work I have done has fixed this situation.

I think this will result in a cleaner implementation, but there are a lot of question marks around whether it can or will work in practice.
Let me know if that makes sense.
--Bill

Most of the relevant comment info is in project_solr_apachesolr_prepare_query() and in the modifications to project_solr_add_sorts().
If you get a chance to take a look at this, I think it'll help our conversations.

This still needs more testing, but this should provide a more streamlined approach to adding sorts and preventing duplication. The only known issue (again, in limited testing. have I mentioned this needs more testing?), is that - without the if statement surrounding the sort adds - the sorts will be added to all searches (even those that aren't project specific). In the grand scheme, this may not be a huge issue, but this could be improved with changes to apachesolr module, so I am going to pursue that as well. This should at least give you a sense of what i'm trying to pull off.
--Bill

Revised versions for when #885950: parse_filters() causes any manually added filters to be lost gets committed.

http://drupal.org/files/issues/881004-6.drupalorg_search.patch has a broken comment:

+  // We filter here based on project since are sorts
+  // are only relevant when sorting based on project.

s/since are sorts/since our sorts/

Same at the top of http://drupal.org/files/issues/881004-6.project_solr.patch

Otherwise, looks great. Vastly cleaner and better than #2.

Thanks!
-Derek

Rerolled with better comments. Otherwise, same as #6.

#885950: parse_filters() causes any manually added filters to be lost was committed to DRUPAL-6--1. So, I committed the patches in #8 to HEAD in CVS of the respective projects, merged into bzr and deployed live on d.o. Working fine:

http://drupal.org/search/apachesolr_search/signup