Integrate with Facet API

As you probably know, I don't have much time at the moment. But in principle integrating with the Facet API would probably be an interesting option. I just haven't looked into it, yet, so I can't really tell how well Search API and Facet API would work together.

So, yes, this is definitely something that should be looked into, some time.

Hi Shadlington.

Thanks for posting this issue. Currently Facet API supports core Search, Apache Solr Search Integration (including Acquia Search), and Search Lucene API (once I get my act together). It would be awesome if Search API integrated with it as well so that people could contribute interesting facet displays that work across all projects. Currently there are a few different search sub-communities which is detrimental to search innovation, so the overall goal is to make sure we are all working together as opposed to coding for specific modules.

It is also important to note that both Apache Solr Search Integration and Search Lucene API stripped out their internal facet building systems in favor of using Facet API. Instead of having to maintain their own facet building code, they now leverage Facet API so they can focus on other tasks. Thomas mentioned that he has limited time, so it might be beneficial to Search API to have one less internal system to worry about.

Thanks again, and I am willing to guide anyone who wants to take up this effort.
~Chris

A corresponding issue has been posted at #1184702: Integrate with Search API.

Sounds like a good direction, I'm up for helping out and if I find time on the weekend to take a look I'll feedback.

If anyone knows how to organize a Bof for Drupal Con London then that sounds like a good way to get everyone working closer. I've just started an elasticsearch module http://drupal.org/project/elasticsearch which I hope will integrate with both Search API and Lucene API.

Elastic Search integration?? Woo-hoo!

Sounds like a good direction, I'm up for helping out and if I find time on the weekend to take a look I'll feedback.

Great, thanks a lot!
Of course, if there are any questions, don't hesitate to ask.

If anyone knows how to organize a Bof for Drupal Con London then that sounds like a good way to get everyone working closer. I've just started an elasticsearch module http://drupal.org/project/elasticsearch which I hope will integrate with both Search API and Lucene API.

I think there's usually a call for BoFs a few weeks before the conference, where you can just reserve a time slot for one.

As my session sadly didn't get accepted, I plan to do a Search API BoF anyways, so that will be a good place to discuss such things. (If this issue isn't resolved by then.) We could then also do a quick sprint right there to get this in. ;)
Will you be in London too, Chris?

I will be in London, Thomas. Would love to catch up and will definitely participate in the BoF. Also, don't feel bad about your Search API session not being accepted, because as far as I can tell no search sessions at all are scheduled. Again, search gets the shaft :-). Looking forward to changing that.

Hey All,
I'm interested in helping out with this. I've had an initial look though the code of both modules to get a sense of how each of them deals with facets and of how much work would be involved. At first glance, it looks like we could do something like this:
- provide one adapter plugin for the entire Search API module which would work for all search back-ends.
- provide Query Type plugins for each Search API based search backend (e.g. search_api_solr), moving all facet-related code out of the main service class and into the relevant plugin class (will presumably require a static caching mechanism to store the current search similar to the one in apachesolr.module)
- get rid of search_api_facets module :-)

Does that sound like a sane overall approach?

Katherine

Katherine,

First off, the Quick Tabs module is one of my projects in Drupal. Amazing work, and thank you.

Second, your approach sounds sane to me. I very much appreciate your interest in this initiative, and please feel free to reach out if you encounter any issues. I am more than willing to hop on IRC / Skype to help resolve issues.

Thanks,
Chris

@Chris - thanks for the kind comments re Quicktabs :-) and for offering to help if I get stuck - hopefully I won't have to take you up on that too often...
@JoeMcGuire - if you're still interested in working on this as well, ping me on IRC so that we can make plan for how to divide up the tasks.
I'm planning to start on this right away as I need it for a client site.

OK, here is a very rough-around-the-edges patch. It achieves the task of getting the two modules to talk to each other, but they are still being a bit shy and reticent :-P
So far I've only tested it with search_api_solr. The good news is that this hasn't as yet required any changes to anything other than search_api module itself. However, in order to get active facets showing up (where they have a '-' beside their name to unset them) is if we get rid of these lines from service.inc in search_api_solr module (around line 650):

          if (empty($facets[$delta]) || count($facets[$delta]) <= 1) {
            unset($facets[$delta]);
          }

And while the changes for the most part are hook implementations and the addition of plugin class files, there are two places where I need to add code to existing search api functions based on whether the facetapi module exists (search_api_menu and search_api_query).

I'm just sticking this up here for now in order to get some expert eyes on it before I go any further - Chris/Thomas if you have a chance I would love your feedback on the general approach. I am sure there will be some WTFs in there as my understanding of both modules is pretty superficial.

Wow, that was fast! Great work!
I don't really know anything about the Facet API, so have to concentrate on the Search API aaspects, but I'll do my best. Maybe together with Chris we can manage a complete review. ;)
And I guess you are aware that there are still a lot of things not working properly, several notices, etc., so won't (further) comment on those either. The base use case seems to work for most fields, in any case.

+++ b/plugins/facetapi/adapter.inc
@@ -0,0 +1,123 @@
+        'limit'             => 10,
+        'display_more_link' => FALSE,
+        'more_limit'        => 10,
+        'min_count'         => 1,
+        'sort'              => 'count',
+        'missing'           => $facet['facet missing allowed'],
+        'show_active'       => TRUE,
+        'default_true'      => TRUE,
+        'ids_list'          => array(),
+        'type'              => '',

I'm pretty sure that most of those won't have any effect in the Facet API?
But as I don't know what this method really does, I could only guess what would be right here.

+++ b/plugins/facetapi/adapter.inc
@@ -0,0 +1,123 @@
+    // Not sure how there could be more than one current search per page request.

There are not only page-based searches, but also some that can be block-based. Or you could have several search-based views on the same page, for whatever reason.
So, ideally, we should somehow identify which search the facets belong to, as the current Facets module does, too.
Of course, I have no idea how to do that, though. There's just the "search id" option for queries that identifies them, but no idea how to use them in the Facet API.

+++ b/plugins/facetapi/adapter.inc
@@ -0,0 +1,123 @@
+    foreach ($keys as $key => $value) {
+      if ($key[0] === '#') {
+        unset($keys[$key]);
+      }
+    }

The keys can be a multi-dimensional array, or a string, too.

+++ b/plugins/facetapi/adapter.inc
@@ -0,0 +1,123 @@
+    // TODO: what to do about the properties of the key array?? '#conjunction' ¶
+    // etc...? I think this is only used for breadrumbs and things though so we ¶
+    // can probably safely ignore.

Ah, that's what the method is for? Then I'd suggest to use the getOriginalKeys() method, which has the keys as entered by the user.
In case of programmatic searches (or other complex cases) these can, too, be an array, but usually they are a string you can just use.

+++ b/plugins/facetapi/adapter.inc
@@ -0,0 +1,123 @@
+    if (isset($searches[$first][1])) return count($searches[$first][1]);

This has to read $searches[$first][1]['result count'].

(Also, please don't use such shortcuts, but the whole if syntax with curly braces.)

+++ b/search_api.module
@@ -149,11 +149,70 @@ function search_api_menu() {
+        // It seems crazy to have to use our own menu callback here when ideally
+        // we would directly use drupal_get_form('facetapi_realm_settings_form').
+        // The problem is we don't want to pass it a loaded Search API Index
+        // object, just the actual machine name. I'm not sure the menu system
+        // has a way of doing that, i.e. telling it not to replace the machine
+        // name with the loaded object for this particular path, but keeping it
+        // part of the same menu :-/

Have you tried the "tab_parent" and "tab_root" keys? I think that's what they are for.
I'm too frightened myself of the complexer uses of the menu system to try, though.

+++ b/search_api.module
@@ -703,6 +762,104 @@ function search_api_search_api_processor_info() {
+  // We basically want to return a searcher for each Search API index, but we
+  // should also store the module responsible for the index (i.e. the module ¶
+  // responsible for the server it's built on) in case we need to invoke hooks.

I don't know what this stored module is used for, but normally it's wrong to say that the module defining the server is responsible for the indexes lying on it. The index can easily switch servers, after all, and should usually not rely on any one server/service class.

+++ b/search_api.module
@@ -703,6 +762,104 @@ function search_api_search_api_processor_info() {
+      $info[$index->machine_name] = array(

Is it really safe to use just the index's machine name as the identifier? In theory, that could clash with other searches, e.g. from the apachesolr module, right?
But that would probably be for Chris to decide.

+++ b/search_api.module
@@ -891,7 +1048,16 @@ function search_api_query($id, array $options = array()) {
+  if (module_exists('facetapi')) {
+    // This is the main point of communication between the facet system and the
+    // search back-end - it makes the query respond to active facets.
+    $adapter = facetapi_adapter_load($id);
+    if ($adapter) {
+      $adapter->addActiveFilters($query);
+    }
+  }

This should go into a hook_search_api_query_alter() implementation, so it is reliably executed for searches. (People might, e.g., create searches by directly invoking $index->query(), or even $index->query().)

I hope I haven't missed any of the questions in code comments? Otherwise, please ask again here.

Powered by Dreditor.

Wow, that was crazy fast!

At first look, the overall direction looks great, and you seem to have picked up the main integration points with Facet API. Nothing really jumps out at me, can't wait up load it up and try it out.

Regarding the caching of hook_facetapi_searcher_info(), Facet API does't do that because it doesn't know when the cache is invalid. Since the backends define the searchers, only they know how they are managed. If performance becomes an issue, I would be willing to add a layer in Facet API that would allow backends to alert when the cache should be cleared.

The FacetapiAdapter::suppressOutput() method confused me as well, but it is something that Peter added so that the Apache Solr Search Integration module could execute a search with no keywords and display the facets in the body of the page for browsing as opposed to relying on the regular facet blocks. That way the search experience could be similar to the Faceted Search module. I think this will be important as the Faceted Navigation for Search module matures and emerges as a D7 replacement for Faceted Search. To see where facets are suppressed in Apache Solr Search Integration, take a look at the apachesolr_search_search_execute() function.

Again, great work!
~Chris

subscribe

subscribe

Wow, thanks for the awesome feedback guys! I will try and get it all incorporated and roll a new patch in the next couple of days. Right now I'm wrestling with the idea of whether Facetapi's notion of a searcher should in fact correspond to search_api's notion of a 'search id' but the more I look at it the more confused I get :-P Maybe I'll fix up the easier stuff first and then see if things have become clearer in my head...

Sticking up a new version of the patch for my own purposes - it deals with some of the issues above but is not yet ready for re-review. I just need to test it in a current build (which uses drush make) so I need it up on d.o.

New patch... this one adopts search_api_facets.module's mechanism for setting facet displays per search page, using the settingsForm() method in the adapter plugin. It also copies search_api_facets' way of keeping track of current searches per facet.
Still need to figure out the other current search stuff in the adapter plugin (getSearchKeys(), getResultCount() etc)...

I've just come to the sad realization that my solution so far completely ignores a major aspect of faceting: the problem of displaying something other than the indexed value as the label of a facet .
As I understand it, the seach_api module handles this by being very entity-centric and storing entity information about all of the fields to be indexed for a particular index. Then it knows how to create a label when it has a value for that particular field - and in the case of facets, this all happens when the facets are about to be displayed.
I'll need to figure out how to make this work with facetapi. In the meantime, Chris, maybe you know how this is generally dealt with, e.g. by apachesolr module?

Working with this stuff can lead to great confusion on my part and I am likely to come out with outrageously incorrect statements about both modules - better to get the confused thoughts out in the open, so that they can be corrected faster :-)

Ooh, I suspect that "map callback" and/or "values callback" will help me here... looking into those (unless Chris tells me I'm on the wrong track).

Yep, map callback solves this, had totally overlooked a whole bunch of stuff in facetapi module. In future I will leave more time before posting here - sorry for the noise!

New patch which adds the map callback stuff (currently only with a mapping for taxonomy terms)

Love reading your progress and thoughts :) As I'm not able to assist you code/api wise, I'll stuck with cheerleading.. go kat go!

Thanks thijsvdanker :-)
This latest version of the patch incorporates changes from the patch at #988276: Support Configurable OR | AND Facets (comment #5). Seeing as facetapi exposes the AND/OR option in its config form I figured we should at least make an attempt to provide it. But while the changes from that patch get the filters to behave correctly, the problems mentioned by Thomas in #7 in that thread still apply: "The big problem is displaying the right choices in the facet blocks. Currently, you would only get those options that would be valid for AND facets and with the AND-specific count."
Still, it's a start.

This latest version of the patch incorporates changes from the patch at #988276: Support Configurable OR | AND Facets (comment #5). Seeing as facetapi exposes the AND/OR option in its config form I figured we should at least make an attempt to provide it. But while the changes from that patch get the filters to behave correctly, the problems mentioned by Thomas in #7 in that thread still apply: "The big problem is displaying the right choices in the facet blocks. Currently, you would only get those options that would be valid for AND facets and with the AND-specific count."

Ah, yes, that's a good idea. And I guess with a change that big (even though it shouldn't really touch the API), we can also make a slight addition to the search_api_facets feature. E.g., an additional key "or" which, when TRUE, tells the server to create an OR facet for that field.
Or a new feature with just that addition? Would certainly be cleaner, as some service classes probably wouldn't support OR facets, and users would then be confused why OR facets are messed up.
Is it possible (without too much trouble) to dynamically decide whether to allow OR facets for a certain index? (Or, is it even possible to disallow them in the first place? Otherwise we wouldn't have much choice …)

Haven't looked at the new patch, yet. Is it still a WIP, or already ready for another review?

(Btw, in case you didn't know: you can use [#NID-COMMENTNO] to link to a comment (where COMMENTNO is the number in the issue, not the cid). E.g., #988276-5: Support Configurable OR | AND Facets, in your case: #988276-5: Support Configurable OR | AND Facets.)

Regarding whether AND / OR facets are allowed, that is set on a per-facet level. You can implement hook_facetapi_facet_info_alter() to turn off OR facets for all facets based on information about the searcher. Concievable this will be the "type", which is usually associated with the entity. It is not named entity because there are cases where a searcher could be associated with a non-Drupal index.

Also, the "searcher" is something unique and usually tied to a search page. For example, Search Lucene API would have one "searcher" per search page, as the facets are enabled per searcher. The name would be something like luceneapi:page-1. A better name would have been "environment", probably.

Great work. I will join in on the cheer. Go, Kat! Go!!!

E.g., an additional key "or" which, when TRUE, tells the server to create an OR facet for that field.

Is this something we could somehow add to the query object? If so it means we won't have to implement the query type plugin per search service module, which I was hoping to avoid. At the point where that code is currently being run we don't know which module will be responsible and have to add its own (e.g. solr-specific) changes to its facet params.

Chris's comments #27 confirm my suspicion in #17, i.e. that the facetapi searcher should not correspond to the search_api index, as I currently have it, but to the particular search page, similar to search API's "search id", but the problem is that this only gets added dynamically, at search time. We need a way to store a list of active searchers, i.e. every time I create a new search page or a new search-based view, this list should get updated. We would need to return this information in our hook_facetapi_searcher_info() implementation. @Thomas - any thoughts on this?

Facets would still be configured on a per-index basis, and although an index is theoretically separate from the service it sits on, it shouldn't be too hard to figure out how to hook in and turn that off when an index is using a service that doesn't support it (though hopefully avoiding having to implement hook_facetapi_facet_info_alter() in the service module itself).

I guess it's ok to review the most recent patch at this stage - the main thing I'm aware of not having been dealt with at all is how the "current search" knows which is the correct current search, but if I get the "search id" per searcher thing figured out then this should be very easy to achieve. The facets themselves should know which search they belong to as I copied the mechanism for doing this from the existing search_api_facets module.

@Thomas - thanks for the tip re linking to comments, I had thought it would work like that but wasn't entirely sure ;-)
@Chris - thanks for the cheering :-)

Is this something we could somehow add to the query object? If so it means we won't have to implement the query type plugin per search service module, which I was hoping to avoid. At the point where that code is currently being run we don't know which module will be responsible and have to add its own (e.g. solr-specific) changes to its facet params.

Yes, I meant as an option on the query object. The requested facets are passed to the server as the "search_api_facets" option on the query object, which is an array of facet fields to return. Each field contains some options for the field (limit, min_count, missing), to which we could add a "or" option.

Chris's comments #27 confirm my suspicion in #17, i.e. that the facetapi searcher should not correspond to the search_api index, as I currently have it, but to the particular search page, similar to search API's "search id", but the problem is that this only gets added dynamically, at search time. We need a way to store a list of active searchers, i.e. every time I create a new search page or a new search-based view, this list should get updated. We would need to return this information in our hook_facetapi_searcher_info() implementation. @Thomas - any thoughts on this?

Huh, this could get complicated …
As you say, the search IDs get created (and can even be altered) dynamically. The current implementation, as you might know, just stores all search IDs it encounters to present the list of available searches to the user. So, before a user goes to a page on which the search is displayed, the Facets module doesn't know about that search.
I don't know how these "searchers" are actually used. If a searcher with an unknown ID would be encountered by the Facet API, would it explode, display the facets, or don't display them? And if we would then add the searcher, once it was encountered – how would this work for the user? Would she have to visit each search page before she can activate facets for it? Or would facets be displayed for all searches by default and couldn't be configured individually until the corresponding search was encountered?

Incidentally, I just solved a similar problem in my new Search API autocomplete module. There, I require modules that define ways to present searches to the users (in the core Search API project, Views and Pages) to implement a hook and offer a function for listing all searches defined by them. So adding a similar hook for facets (or generalizing that hook and moving it into the Search API core module) would also be a possible way of resolving this – but, of course, not really an ideal one.

If a searcher with an unknown ID would be encountered by the Facet API, would it explode, display the facets, or don't display them? And if we would then add the searcher, once it was encountered – how would this work for the user? Would she have to visit each search page before she can activate facets for it? Or would facets be displayed for all searches by default and couldn't be configured individually until the corresponding search was encountered?

For now I've implemented it the same way as it works in the original search_api_facets module. Your "default_true" option is either "show for all searches but those listed" or "show for only those listed", defaulting to the first. So yes, new searches that facetapi didn't yet know about at the time of configuration would automatically get the facets if this is still set to the first option.

Your solution in Search API autocomplete sounds like exactly what we would need, and I would definitely advocate generalizing it and moving it into Search API core. What are the problems you see with it?

For now I've implemented it the same way as it works in the original search_api_facets module. Your "default_true" option is either "show for all searches but those listed" or "show for only those listed", defaulting to the first. So yes, new searches that facetapi didn't yet know about at the time of configuration would automatically get the facets if this is still set to the first option.

That sounds pretty good to me already. I wouldn't think it worth the trouble to invest energy here just so we have all searches right from the start.

Your solution in Search API autocomplete sounds like exactly what we would need, and I would definitely advocate generalizing it and moving it into Search API core. What are the problems you see with it?

It is an API addition, and moves facets away from their general plug-and-play approach working for all searches to something that module developers would have to explicitly support. I just think it isn't worth the effort. Also, there haven't been any complaints about the current system, even though it probably isn't optimal from a UX point of view.

Hmm, I guess I was seeing it as a non-facet-specific API addition (i.e. because it would be used by other features such as the autocomplete module and maybe others in the future) which might therefore make it worthwhile.
If not I guess we need to figure out what the implications are for this patch of having hook_facetapi_searcher_info() just return information for each index rather than for each search page.

Hmm, I guess I was seeing it as a non-facet-specific API addition (i.e. because it would be used by other features such as the autocomplete module and maybe others in the future) which might therefore make it worthwhile.

The autocomplete feature couldn't really use this, as it a) also has other requirements for implementing modules and b) only needs those searches with a fulltext search field.
There might be other modules that could use this, but I don't really see a case where this would really be necessary, in such a general form. And in that case it's just an (hopefully) unnecessary burden for people implementing some search with the Search API and then wondering, why there aren't any facets.

If not I guess we need to figure out what the implications are for this patch of having hook_facetapi_searcher_info() just return information for each index rather than for each search page.

Ah, sorry – the question in #29 (which you answered in #30) was targetted at the possible implementation of search IDs as different searchers, not the current one (if that still uses indexes as searchers – sorry, I'll look at it over the weekend). So that wouldn't be possible (or at least not so well) without being able to obtain all searches right away?
Then we should really think about this. Implementing yet again a parallel system, just for dealing with enabling/disabling facets per page, doesn't seem like a good idea when we are finally embracing the Facet API to take care of such generics for us. Of course, I'm only slowly getting to understand the Facet API module, so can't really tell what the consequences of either implementation would be. Or how/whether we could have per-page options for facets, but won't have to have all searches available right away. (So, have some default settings per index, and after encountering a search you also get the chance to override them. Or something like that.)
Maybe Chris can enlighten us here (again)?
And, what are those "realms" – could they maybe be used for something like this? (By the way: where can I find some documentation on the Facet API?)

OK, to be really clear about the issues we're discussing and to avoid future confusion, here they are as I see them:
1. As you put it:

how/whether we could have per-page options for facets, but won't have to have all searches available right away

I think we are agreed that this is dealt with satisfactorily, using the mechanism you had written for search_api_facets and which I reworked into the facetapi patch.

2. Telling facetapi about the searchers provided by search_api module. Currently I am just returning information for each index, but of course each index could be used for multiple searchers (search pages, search-based views, etc). The main consequence I'm seeing of this, is that we have no way to match a "current search" to its (facetapi) "searcher".

The main consequence I'm seeing of this, is that we have no way to match a "current search" to its (facetapi) "searcher".

Ah, OK, then I still misunderstood before.
So, without having individual search IDs (each view, each page, …) as facetapi "searchers", you see no way of dealing correctly with multiple "current search"es on a single page?
This would be a restriction, but only a small one. Does Facet API even support facets for multiple searches on a single page? If not, then this is rather moot point anyways. People will just have to enable facets for one of the searches displayed on a page, and then there will be only one "current search" with a fitting search ID.
And even though we were previously able to contruct cases where having individual facets for multiple searches on the same page might make sense, this is really not a use case over which we should rack our brains. Usually, if there is more than one search on a page, only one will be a "real" search, and the others things like a "More like this" block, a "Facets block" view, internal queries for range facets, etc., for which facets only make sense in the rarest of cases.

(I hope I now finally understood the problem here.)

Yes, I think this may indeed be less of a problem than I'm even making it out to be: I think it is only a problem for the "current search" block itself, i.e. the one that displays info about how many results it found, not for the actual facet blocks, because they DO know about search ids, from their config settings.

Ah, right, the "Current search" block … That one could indeed be a bit problematic, without resorting to some complex magic (investigating for which of the current searches there are enabled facets, or taking the search with fulltext keywords (if exactly one is present)).
Are there no searcher-specific settings for the "Current search" block available? (In that case, I'd of course love to shout "Feature request!", now that I wouldn't have to implement it. ;P)

New patch which adds the date query type plugin. I copied a huge swathe of code from search_facetapi module's implementation, which strongly suggests it could use some abstraction.

Also, there was a problem with the default facets as defined in facetapi module itself and the ones being created automatically for every field in a given Search API index. If two had the same name, e.g. 'created' (for faceting on the node creation date), the facet name would end up as an array due to module_invoke_all() being used to get all the facet definitions. I've submitted a patch to facetapi to get around this for now (#1202022: Don't use module_invoke_all in facetapi_get_facet_info()) but as I mention there it seems like facetapi should not be responsible for providing default facets as it is making assumptions about the fields being indexed. For example, it provides a "language" facet - but I may not be including the language field in my index. I know that apachesolr module indexes these fields (language, uid, created, updated) and you don't have control over it but in Search API you do have control and can chose not to index these fields.

So, in this patch I've also implemented hook_facetapi_facet_info_alter() to unset facets that were not added by Search API itself. Obviously this is not the ideal way to be doing this, it's just a workaround for now, but I'd like to get Chris's feedback on this problem.

Just realised that Search API's related fields, e.g. "author:name", don't work so I'm skipping over them for now and adding a TODO for dealing with them.

And another fix :-P The date-based facets were causing errors when there were no search results...

Sorry for taking so long for the review. And now I didn't even manage to review the code, I hope I'll get to that tomorrow. Just one thing:

+++ search_api.module
@@ -707,6 +766,135 @@ function search_api_search_api_processor_info() {
+      if ($disabled = empty($index->enabled)) {
+        drupal_set_message('Since this index is at the moment disabled, no facets can be activated.', 'warning');
+      }

I think this misses a return statement. If you view the "Facets" tab on a disabled index, you get rather ugly errors.

The other problems I found, by only trying the patch out:

First off, the date facets rock! Really cool how you got those working.
However, I guess this is really just a workaround for fixing them in the backends … Like this, I think they'll only work correctly when the facet limit is high enough to contain all items?
Also, there were problems both in the Solr and the DB backend. In Solr, when few items were displayed, some errors started to show and the date facet wouldn't be displayed anymore (I think when there is only a single facet value):

    * Warning: ksort() expects parameter 1 to be array, null given in SearchApiFacetapiDate->build() (line 88 of search_api/plugins/facetapi/query_type_date.inc).
    * Warning: array_keys() expects parameter 1 to be array, null given in SearchApiFacetapiDate->build() (line 112 of search_api/plugins/facetapi/query_type_date.inc).
    * Warning: Invalid argument supplied for foreach() in SearchApiFacetapiDate->build() (line 124 of search_api/plugins/facetapi/query_type_date.inc).
    * Warning: Invalid argument supplied for foreach() in SearchApiFacetapiDate->build() (line 141 of search_api/plugins/facetapi/query_type_date.inc).

On DB servers, however, I could drill down right to a "1:17 PM" facet, but when clicking on that I got an "Allowed memory size exhausted" fatal error, in modules/facetapi/plugins/facetapi/adapter.inc on line 1230.

For search views, the breadcrumb for the pure keyword search doesn't work. (It's the same link as for the normal view without keys.) Don't know how easily this could be fixed, though. Or even, whether the bug is in the integration or in the Facet API itself.
To me it seems like you'd just have to remove all known facet filter GET parameters, but leave the others. (Currently, it apparently removes all GET parameters.)
But I guess that's really a bug in the Facet API itself.

Just realised that Search API's related fields, e.g. "author:name", don't work so I'm skipping over them for now and adding a TODO for dealing with them.

What's the problem with them? Doesn't the Facet API allow colons in names? If so, shame on you, Chris! ;P

Powered by Dreditor.

OK, now here the complete code review:

First off, I noticed the hook_menu() wasn't changed. Didn't "tab_parent"/"tab_root" work for you, or didn't you try them, yet? search_menu() and user_menu() have examples in core, from the latter I guess that the following should work:

$items[$pre . '/index/%/facets'] = array(
  'page callback'    => 'drupal_get_form',
  'page arguments'   =>  array('facetapi_realm_settings_form', 5, $realm_name),
  'tab_parent'       => $pre . '/index/%',
  //...
);

+++ plugins/facetapi/adapter.inc
@@ -0,0 +1,180 @@
+    $options = $facet_settings->settings;

Line occurs twice.

+++ plugins/facetapi/query_type_term.inc
@@ -0,0 +1,103 @@
+        if ($operator == '!') {

I think $operator here is a setting for the whole facet, and either "and" or "or". You want to check whether the facet term ($filter_array['value']?) equals "!".

+++ plugins/facetapi/query_type_term.inc
@@ -0,0 +1,103 @@
+          $filter = trim($filter_array['value'], '"');

Please use substr() here, like I did in the Facets module, as the filter value itself might, in theory, also start/end with a quote.

Also, this code doesn't seem to take ranges into account?

+++ plugins/facetapi/query_type_term.inc
@@ -0,0 +1,103 @@
+        if ($operator == '!') {

Same as above.

+++ plugins/facetapi/query_type_term.inc
@@ -0,0 +1,103 @@
+        $query->filter($facet_filter);

This line should be outside of the loop.

+++ plugins/facetapi/query_type_term.inc
@@ -0,0 +1,103 @@
+      $values = $results['search_api_facets']{$this->facet['field']};

Why do you suddenly use curly braces for array access here? Until now I didn't even know this would work – the PHP docs also don't mention this as far as I can see. So please change back to brackets.

+++ plugins/facetapi/query_type_term.inc
@@ -0,0 +1,103 @@
+          $filter = trim($value['filter'], '"');

As above. Also, I'd think you also want to discern here between "value", ! and [lower upper]?

+++ plugins/facetapi/query_type_date.inc
@@ -0,0 +1,160 @@
+    $range = array(
+      $this->facet['min callback']($this->facet),
+      $this->facet['max callback']($this->facet),
+    );

$range doesn't seem to be used anywhere.

As for the rest of the function, apart from the same things already mentioned for the other query type, I won't even try to understand what's going on. I'll just assume that it's good, as long as it works (which it doesn't, as mentioned in my previous comment, but just generally speaking …).

+++ search_api.module
@@ -707,6 +766,135 @@ function search_api_search_api_processor_info() {
+      $info[$index->machine_name] = array(
+        'label' => t('Search service: @name', array('@name' => $index->name)),
+        'adapter' => 'search_api',
+        'instance' => $index->machine_name,
+        'path' => '',
+      );

- I still think the key should be prefixed with "search_api:" (or similar). The apachesolr module does this, too. You can use the "instance" key to get the machine name (or just do a substr()).
- Based on Chris' comment above, 'type' => $index->entity_type should probably be added.
- Why is the path empty? Would that lead to some unwanted magic happening?

@ Chris: What exactly can/should go into this hook?

+++ search_api.module
@@ -707,6 +766,135 @@ function search_api_search_api_processor_info() {
+      // TODO: figure out if there are other callback mappings needed for other
+      // entity types. These are what take care of replacing the indexed value
+      // (such as a tid) with the label (e.g. the term name) for facet display.
+      $field_map_callbacks = array(
+        'taxonomy_term' => 'facetapi_map_taxonomy_terms',
+      );

Well, this would basically be needed for all entity types. In theory, this could be a simple generic function, using entity_load() and entity_label().
However, it seems there is no way to get the entity type to the callback function, right? This would have to be worked out, as having facets not work with, e.g., users anymore would be too much of a regression.

@ Chris, if my above assumption is right: is there a chance we can add some context parameter (facet definition) to the callback function?

+++ search_api.module
@@ -707,6 +766,135 @@ function search_api_search_api_processor_info() {
+        if (empty($facets[$key])) {

This check doesn't make sense to me. First off, $facets contains data of indexes, not of fields, so checking whether the field ID is present there doesn't make sense. Secondly, if you meant $facet_info, I don't see a way the key could already be set, as you only copy the keys from another array.

+++ search_api.module
@@ -707,6 +766,135 @@ function search_api_search_api_processor_info() {
+              'min callback' => 'facetapi_get_min_date',
+              'max callback' => 'facetapi_get_max_date',

These are hardcoded for nodes, so can't be used here.
However, you don't seem to need them, anyways, so I think they can just be removed.

Also, please look out for trailing whitespace. My git always yells at me when applying your patches. :-/

Powered by Dreditor.

Thomas,

First, facet names should be able to accept colons. If not, I will join in the shaming of me :-).

Regarding the empty $path, Facet API adds menu items based on that path. If it is empty, then you can add your won menu patch which is useful for Search API and Apache Solr Search Integration. In terms of what goes in that hook, it is listed below:

function mymodule_facetapi_searcher_info() {
  // Keyed by a unique ID for the search page.
  $info['mymodule:' . $some_unique_id] = array(
    // Human readable name of the searcher.
    'label' => t('Apache Solr environment: @environment', array('@environment' => $environment['name'])),
    // ID of the adapter plugin defined in hook_facetapi_adapters().
    'adapter' => 'apachesolr',
    // The "type" of content being indexed, usually the entity id.
    // NOTE: Issue 1167974
    'type' => 'node',
    // An optional path the the admin page.  If passed, Facet API will add menu items for admin pages.
    // @see the Faceted Navigation for Search for where a path is passed.
    'path' => '',
    // Whether the search page supports the Solr concept of a facet item for "missing", i.e. not in the index.
    'supports facet missing' => TRUE,
  );

  return $info;
}

Regarding additional options being passed to the mapping callback, this is done through the "map options" key. See the apachesolr_facetapi_facet_info() hook implementation for an example of this.

Hope this helps, and let me know if I missed any questions,
Chris

Hope this helps, and let me know if I missed any questions,

I think those were all for the moment, thanks!

Ah, also, @ Katherine: Please remind me that we should add some migration handling to the search_api_facets module once this issue is about cleared.
At the moment I plan to add some "deprecated" notes (.info description, README, project page, release notes) and a function (plus maybe primitive UI) for migrating existing facets to the Facet API ones. (Just have to look up how you programmatically create those.) Then, after a few releases, I would remove the Facets module and hope that it doesn't cause too much havoc anymore.

I haven't replicated the issue, but I think I understand why facets with colons in their names aren't working. Boooo, Chris, boooo! #1209490: Facet names with colons don't work

Ah, OK, good to know. Thanks for investigating!

This patch addresses (I think) all of the WTFs in the previous patch, as discovered by Thomas (curly braces? Clearly, I was on crack). Unfortunately it does not address all of the outstanding problems. I'll try to outline here what remains:

Ranges

Also, this code doesn't seem to take ranges into account?

Nope, I don't know how to address that and will have to leave it for someone else or at least get some guidance.

Also, I'd think you also want to discern here between "value", ! and [lower upper]?

This is basically the same point, right? i.e. we need to deal with range queries. Again, need guidance.

Date-based facets

I'll just assume that it's good, as long as it works (which it doesn't...

Alas, it is mostly voodoo to me as I copied it in its entirety from the implementation in search_facetapi. I've made a couple of changes that get rid of the notices you mentioned above. It seems to work ok for me now but as I say I am really unfamiliar with what should be happening here.

Searcher keys

I still think the key should be prefixed with "search_api:" (or similar). The apachesolr module does this, too.

If we do this, then we definitely need to implement our own menu callback function for the facet settings pages, rather than using the solution you outlined above with 'tab_parent' and directly calling the 'facetapi_realm_settings_form' from the menu item. The reason for this is that the first paramter to that form function must be the searcher key, which rather than simply being the index machine name, would now be 'search_api@[index_machine_name]' or whatever. By the time I had realised this I had already made the change in hook_menu, so I've left it as is, but if you're ok with going back to using our own menu callback function I will implement it similar to what they've done in apachesolr module.

Current Search

I've added new comments in the methods in adapter.inc that pertain to the current search and breadcrumb functionality, which is still problematic. The main problem is what Thomas mentions in #37, i.e. no searcher-specific settings for the current search block. I wonder if the changes envisioned here would help: #593658-4: Make the current search block more configurable. On the other hand, this also affects the breadcrumbs functionality, so probably needs more thought.

Some miscellanous stuff...

I've responded about the facet names with colons in the facetapi issue.

Please use substr() here, like I did in the Facets module

For some reason, in query_type_term.inc the values are no longer coming through with quotes around them and I'm not sure if that's due to some other change I made elsewhere... for now I'm just going with the value as is (without trim or substr) until I find out why this changed.

It's a bit late here now to review properly, but here is at least a response to the comments:

Nope, I don't know how to address that and will have to leave it for someone else or at least get some guidance.

Guidance about the Facet API or the Search API part?
Regarding the Search API, it's pretty easy (I hope). Facet values can come back from the server in one of three ways:
- "VALUE": Means the value is used as a normal filter, field = VALUE.
- [VALUE1 VALUE2]: Stands for values in the range specified by the two values which are split by a space – field >= VALUE1 AND field <= VALUE2. The special value * for one of the two stands for "unbounded", so, e.g., [* VALUE2] would mean field <= VALUE2. Also, parentheses instead of brackets indicate the limit is not inclusive: [VALUE1 VALUE2) means field >= VALUE1 AND field < VALUE2.
- !: This stands for a facet for a missing value, field = NULL.
What you have to make of that will then depend on the context.

Alas, it is mostly voodoo to me as I copied it in its entirety from the implementation in search_facetapi. I've made a couple of changes that get rid of the notices you mentioned above. It seems to work ok for me now but as I say I am really unfamiliar with what should be happening here.

Well, this should really be done on the server side anyways, so unless it throws any notices/errors, I guess a workaround is better than nothing. However, this should definitely take ranges into account, so if a server correctly returns date ranges, we won't mess them up again.

If we do this, then we definitely need to implement our own menu callback function for the facet settings pages, rather than using the solution you outlined above with 'tab_parent' and directly calling the 'facetapi_realm_settings_form' from the menu item. The reason for this is that the first paramter to that form function must be the searcher key, which rather than simply being the index machine name, would now be 'search_api@[index_machine_name]' or whatever. By the time I had realised this I had already made the change in hook_menu, so I've left it as is, but if you're ok with going back to using our own menu callback function I will implement it similar to what they've done in apachesolr module.

Ah, OK, then we should definitely do this, yes. And the @ is fine as the separator, if apachesolr uses it too. Just put the menu handler into the .admin.inc file, please.

For some reason, in query_type_term.inc the values are no longer coming through with quotes around them and I'm not sure if that's due to some other change I made elsewhere... for now I'm just going with the value as is (without trim or substr) until I find out why this changed.

Uh, OK …

Bugs in the UI I (still) came across:

- If I click on an author facet, the "Current search" block shows the UID, not the name. For taxonomy term facets it seems to work, though.
- Is it possible to display the corresponding field names along with the active facets in the "Current search" block? (Probably more a question to Chris than to Katherine …)
- #1210466: Search keys breadcrumb doesn't work if they are in a GET parameter
- Clicking on the "Facets" tab of a disabled index still leads to the fatal error. @ Chris: What do you think we should do when indexes get disabled, or moved from a server supporting facets? Hm, maybe we can just do this in the new menu callback, if we need one anyways?

+++ search_api.module
@@ -707,6 +748,141 @@ function search_api_search_api_processor_info() {
+            // Our entity map callback will need to know more about the field,
+            // in particular, what entity_type it is.
+            $map_options = $field;
+          }
+        }
+        $facet_info[$key] = array(
+          'label' => t('!field', array('!field' => $field['name'])),
+          'field api name' => $field['name'],
+          'description' => t('Filter by @type.', array('@type' => $field['name'])),
+          'map callback' => isset($map_type) ? $field_map_callbacks[$map_type] : '',
+          'map options' => isset($map_options) ? $map_options : array(),
+          'allowed operators' => array(FACETAPI_OPERATOR_AND => TRUE),
+        );

This code will use the $map_options of previous iterations of the loop in following ones.

Otherwise (and apart from the thing with ranges and the "missing" facet value), this looks quite good already.

Powered by Dreditor.

OK, I've reworked it to use searcher keys prefixed with 'search_api@' and added the required menu callback to the admin.inc file. The menu callback deals with disabled indexes by just returning a message to this effect. I removed these lines:

    if ($disabled = empty($index->enabled)) {
      drupal_set_message('Since this index is at the moment disabled, no facets can be activated.', 'warning');
      return array();
    }

from search_api_facetapi_facet_info() because I don't think this is necessary.

I haven't managed to figure out why the current search block is unable to map the UID (and presumably entity IDs in general, even though there is a map callback). I'm hoping others will start testing out the patch and might be able to help with the outstanding issues as I'm not sure how much more time I'll be able to spend on it.

+++ search_api.admin.inc
@@ -1970,3 +1970,17 @@ function search_api_facet_settings($realm_name, $search_api_index = NULL) {
+  if (!$search_api_index->enabled) {
+    return array('#markup' => t('Since this index is at the moment disabled, no facets can be activated.'));
+  }

This should also check for $search_api_index->server()->supportsFeature('search_api_facets'). (And use SearchApiIndex $index for the parameter, to be consistent.)
Also, when you don't allow the index to be NULL, don't include that in the parameters. Either remove the = NULL part, or (if this can really happen) check for it in the function body before accessing other properties.

Powered by Dreditor.

To come back to Kat's points in #47...

Date-based facets

Just to clarify, the "query type" plugin is responsible for two things. One is to append the facet query to the backend's query object via the FacetapiQueryType::execute() method, and the second is to build the facet's render array from the response returned by the backend in the FacetapiQueryType::build() method. That's really all the build() method is doing in the date query type plugin, however with dates it is a little complex to get from point A to point B. The best example to work of off is actually in the Apache Solr Search Integration module. As long as the backend returns only the values you need, you don't have to do a lot of processing like the Faceted Navigation for Search module has to do. I actually forked a lot of that code from version 6.x of Apache Sole Search Integration, so it is voodoo to me as well. However it seems to work across all backends that Facet API supports, so it is apparently good voodoo :-).

Ranges

Ranges will simply be a query type plugin without passing facet parameters. Hopefully Peter or I can whip something up for Apache Solr or Faceted Navigation for Search respectively so some example code can exist.

Current search

My hopes with the Current Search block is to be able to configure experiences like Amazon and Endeca. You will be able to enable and disable various elements, and you will be able to token replace strings to format messages like "Showing x out of x" and things like that. Not sure if I am missing the concerns, but I wanted to give a roadmap of what I was thinking.

Again... great work,
Chris

Pardon my sloppiness ;-) (addresses #51)

Current search

My hopes with the Current Search block is to be able to configure experiences like Amazon and Endeca. You will be able to enable and disable various elements, and you will be able to token replace strings to format messages like "Showing x out of x" and things like that. Not sure if I am missing the concerns, but I wanted to give a roadmap of what I was thinking.

The main concern at the moment is (if I'm not mistaken) that there could be more than one search with the same index (= searcher) and both Kat and I have no idea how we could know for which one we should display the "Current search" block. For facets we use custom settings, but there doesn't seem to be such a thing for the "Current search" block.
Letting each individual search be a "searcher", which seems to be the normal solution, was considered less practical than the current solution, for several reasons.

And regarding ranges, I think she didn't mean the "Search API ranges" module (which indeed has to be adapted somehow) but the internal concept in Search API's facets, in which a server can return not only fixed values for facet terms, but also ranges, which the current code doesn't wholly take care of at the moment. I don't know where Kat's concrete problem lies with them, though – maybe she'd need an explanation from you about what she could do about them.

@ #53: Thanks!
Although I forgot to mention that I also wondered whether there's a special reason why you nest the returned form in another array?

So the reason for the nesting and being able to configure the "key" is to group facets when they are rendered as form elements. I haven't ported the "fieldset" realm from D6, but once that is done it will facilitate having a taxonomy search similar to the core search module's implementation. If facets have the same "key", then they can be grouped in a single form element. It might be a relic as we have an opportunity to rethink and redesign the advanced search form, however that was the use case and it doesn't hurt to leave it in.

@ #56: Is this in response to #55? I was talking about the additional nesting in Katherine's search_api_facet_settings() function – I don't think that was what you meant?

In any case, I don't completely understand what you mean, but this could be highly relevant for my GSoC project, so thanks for the pointer! (Hum, I haven't given any thought to what to do with that task now, anyways …)

I am trying to think if Facet API would support multiple searchers on a page, and I believe that the architecture is there although the module may or may not need a patch, leaning towards not. Right now you can set the facet parameters through the adapter via the FacetapiAdapter::setParams() method, so the second search would simply have to supply the params manually as opposed to pulling directly from the query string. I am starting to think it would be a good idea to specify which query string variable the facets are stored in in the searcher_info hook, defaulting to "f" to maintain backwards compatibility.

Each searcher will have a separate adapter instance, and each searcher has their own current search block as well. Therefore if both adapters said "I executed a search", I don't think there is anything stopping you from displaying both blocks. The issue would be trying to squish both in the same block. The problem I see is with the link generation, which again wold hard code "f" as the query string parameter the active items are stored in. Not a huge modification at all to change this, which would probably be a good idea anyways.

If you are trying to have two of the same searches on the same page, that might a little trickier as the adapter implements the singleton pattern. Therefore only one adapter per searcher can be instantiated. I am not sure why someone would want two of the same searches on the same page, but maybe you have a use case I haven't thought if which is entirely possible.

Thanks,
Chris

@#57, yes, #56 is in response to #55. No, I was not referring to search_api_facet_settings(). I must have completely misunderstood the post.

If you are trying to have two of the same searches on the same page, that might a little trickier as the adapter implements the singleton pattern. Therefore only one adapter per searcher can be instantiated. I am not sure why someone would want two of the same searches on the same page, but maybe you have a use case I haven't thought if which is entirely possible.

The problem is that you could have the same index powering two different searches on the same page - one being the "main" search, and the other being, say, a block that displays some stuff that happens to be search-powered. The block search does not need facets or current search and would not need the adapter instantiated. But because we are using the index and not the actual searcher as the facetapi searcher info key, we can't tell, when building the current search stuff, which current search to base it off of.

I think that's it anyway, though I'm liable to get confused... :-S

I think that's it anyway, though I'm liable to get confused... :-S

I think so, too, so don't worry. ;) (Working from the premise that there's always at least one of us not confused …)

I think what I am trying to say is that multiple "searchers" can query the same index, but be different in Facet API's eyes.

P.S., I always feel confused, so in a way, I am never confused. It's very confusing.

+1

As Chris explained here, we should set a "hierarchy callback" for all taxonomy term facets. A great effect for just about three lines of code.

As is also pointed out there, this issue will in effect complete one of my GSoC tasks. Therefore, Katherine, you don't have to worry about the issue getting stuck when you can't invest any more time – I'll have to make sure this issue gets completed until mid-August, so would then just have to do more than merely nagging at you. ;)

OK, well of course I couldn't resist attempting the hierarchy callback thing and have added it in this latest patch (I've reworked the logic for adding these various field-type-specific faceting options, I hope it's a bit cleaner and easier to follow).
However, it's not quite such an easy win, as some work needs to be done on the indexing side. From apachesolr module's term reference indexing callback function:

      // By including the ancestors to a term in the index we make
      // sure that searches for general categories match specific
      // categories, e.g. Fruit -> apple, a search for fruit will find
      // content categorized with apple.

Currently, with Search API, a search for "fruit" will only deliver results that were actually categorised as "fruit", not those that were categorized with any of its subterms. Which means of course that we don't get to see the lovely child facets in action. Sad panda :-(

Also, Thomas noted in #49:

If I click on an author facet, the "Current search" block shows the UID, not the name. For taxonomy term facets it seems to work, though.

I'm seeing taxonomy terms as integers in the current search block as well, i.e. the mapping callbacks aren't being used, even though they work fine on the facet blocks themselves. This is a really tricky one to try and figure out so Chris if you have any insights as to what might be going wrong here that'd really help.

Currently, with Search API, a search for "fruit" will only deliver results that were actually categorised as "fruit", not those that were categorized with any of its subterms. Which means of course that we don't get to see the lovely child facets in action. Sad panda :-(

Ah, damn, you're right of course …
OK, then we maybe just need a system for defining an entity type as "hierarchical" for the overall Search API. I'm pretty sure this will be rather easy to add, and will do so before the patch gets committed. (Ah, there goes my effortless task …)

Hm, or maybe rather a data alteration for that purpose? Is there a use case for having only the directly set terms in the index, and not their parent terms?

OK, then we maybe just need a system for defining an entity type as "hierarchical" for the overall Search API. I'm pretty sure this will be rather easy to add, and will do so before the patch gets committed. (Ah, there goes my effortless task …)

Disregard that – when displaying the facets in their flattened form, I guess indexing the parents wouldn't be desirable for the user, so we definitely need to leave a choice.

Therefore, I'd say we solve this with a data alteration. Which I can implement completely independt of this issue – we should just leave the "hierarchy callback" in there for now, even if the results aren't correct right now.

See #1213698: Add a data alteration for indexing complete hierarchies.

@#65 Hmm, that obviously shouldn't happen. The mapping callbacks only get invoked once so the Current Search block doesn't have to use re-calculate them. I am wondering if there is some ordering issue with Facet API + Search API that is rendering the Current Search block prior to those values being calculated. Seems weird to me, though, because both the FacetapiAdapter::buildRealm() and FacetapiAdapter::buildCurrentSearch() methods call FacetapiAdapter::processFacets(), so no matter which method gets called first the facets are "processed", which included their values being mapped.

The flow is a little confusing, but the FacetapiAdapter::processFacets() instantiates a FacetapiFacetProcessor object. Then it calls FacetapiFacetProcessor::process() which does the value mapping, hierarchy processing, query string building, etc. The kicker is that it only does this once, so it the facets aren't being mapped for the Current Search block then they shouldn't be mapped in the actual facet block either. Are you using Facet API's current search block for Search API's?

Could this be caused by servers not returning facets that are already active (i.e., what Kat mentioned in #12)? I.e., does Facet API automatically add all active facets to the available ones and processes them with the others, or do they only get added later, and then possibly without mapping?

I don't want to make any assumptions, but I am not sure this is it. Facet API does a pretty good job at figuring out which facets are active / inactive as well as which ones are enabled. What might be happening here is that the parent's are marked, for example if "apple" is a child of "fruit", Facet API will expect that when "fruit" is active that "apple" is active as well. Since Search API isn't indexing that relationship yet, it **could** be where the issue is stemming from. Nothing jumps out at me though, so I am going to have to actually run the code provide any useful guidance.

I don't want to make any assumptions, but I am not sure this is it. Facet API does a pretty good job at figuring out which facets are active / inactive as well as which ones are enabled. What might be happening here is that the parent's are marked, for example if "apple" is a child of "fruit", Facet API will expect that when "fruit" is active that "apple" is active as well. Since Search API isn't indexing that relationship yet, it **could** be where the issue is stemming from. Nothing jumps out at me though, so I am going to have to actually run the code provide any useful guidance.

As said, this also happens for authors, so I don't think it has got anything to do with relationships.

Makes sense. You are probably right, then. Since it is happening with authors, it is probably not because of the relationships and there is probably some assumptions made in the order that Facet API is processing things. Hopefully I will have some time to run through the code to help resolve the issue.

Thanks,
Chris

Just FYI I started a sandbox project for this at http://drupal.org/sandbox/cpliakas/1215178. Kat and Thomas, you both have write access to the repository. The project is set up to track Search API, so any commits made to the core module can be easily merged back into the sandbox project. This is pretty much the same idea as http://drupal.org/node/1181472.

I feel that patches are a tough way to work, and this way Kat can commit without having to post the patch every time, just a reference to the commit. We can do a git pull then a git diff 7.x-1.x to see the patch file. Might make things a little easier.

If you want to go the sandbox route, I created a facetapi branch that we should work off of. I committed Kat's last patch, so it should be up-to-date with everything that has been done so far.

Thanks,
Chris

Thanks Chris, great idea! I actually have something similar set up locally, but it of course makes more sense if we can all work directly with the repository.

Yay - I had been thinking of suggesting a sandbox for this - thanks for setting it up! Already made a tiny commit to it (just a small fix for date facets).

Great! Commit away. I'm not sure if it would help to start tracking some issues there since we have multiple threads of conversation, but I am going to throw a couple up to see if it helps, hurts, or makes no difference. Started stepping through the code in a debugger and found a couple of issues. So far I am very impressed with the progress and look forward to contributing.

#1215526: Add support for the "Bundle" dependency plugin
#1215520: The "field api name" key should only be set for Field API fields, not all properties

It might really be a good idea to use separate issues, as this helps keep track of which problems are still present. Maybe I should create issues for the other open problems, too … In any case, I subscribed to the queue there, so it's practically all the same to me where we continue the discussion.

Oh, and by the way:

This is pretty much the same idea as http://drupal.org/node/1181472.

Thanks for the link! As you might know, I plan to do some major core development in the near future. ;)

Core contributions, huh? Good luck! I tend to stay away from core, as I feel that I can get more done in contrib :-).

Tagging, as per webchick's orders. *salutes*

subscribe

According to #1167974: Allow for multiple types to be associated with a searcher the key type in hook_facetapi_searcher_info() is deprecated in favour of types.

Hi das-peter.

Thanks for the contribution! Would you mind posting an issue and patch over at http://drupal.org/sandbox/cpliakas/1215178? This is where the work is happening, and we are leveraging the sandbox's issue queue to track what needs to be done in terms of the integration.

Thanks again,
Chris

By the way, to use this issue for discussing the whole task: How far are we?
The dependency system and maybe the OR facets need some work. What's your opinion on the other issues? They mostly need changes in the Facet API (or consist entirely of Facet API changes) – would you say we could commit this even though they aren't fixed yet, and jjust fix them later, when Facet API is ready? Or just wait for them, as we're in no hurry? (Or, is anyone?)

And another thing: How should I commit this eventually? The new guideline is to attribute directly through Git's "Author" field, but that doesn't support multiple authors (as far as I know – and which I consider quite a major drawback). So, should I just use Kat (I think we can agree she did the main work) as the author and mention everyone in the message? Or what's the usual procedure for such issues?

@drunken monkey:
I know I am not participating actively here, but just to make a suggestion on how you can commit this.

Actually you do not need to make one commit, and as far as I could see on the sandbox repository, you have the right authors on that repository. I have also seen that you have been merging facetapi branch on with 7.x-1.x already. So you just need to merge to your 7.x-1.x with the facetapi topic branch when the time is right, and push your changes to the upstream repository instead of pushing to the sandbox repository. In that way, you preserve authorship on each commit on the topic branch.

I hope I have said something new to you, if not, just ignore this ;-)

Re #84, I am hoping to resolve the issues this weekend, however there might be a couple of outliers. I think the main issues have been addressed, and I feel comfortable rolling out a release once the filters stuff is fixed. Maybe we should target a merge by DrupalCon? Seems like a resonable timeline, and maybe we can pound out some of the remaining stuff in person during a sprint.

Very excited about this, and very impressed with the work you guys did to move this along.
~Chris

subscribing. I need better facets for search api and after playing around with ApacheSolr + FacetAPI...i'm almost tempted to drop Search API.

Looking forward to progress with this patch.

@drunkmonkey. I for one want this committed ASAP and am willing to test & debug. For me, Search API is "broken" currently due to the lack of useful facets. I'd prefer to have better ones integrated, even if they didn't work 100%.

Subscribing... and here's a pat on the back for you people doing the hardyards so we can reap the rewards. Appreciate it.

I'd like to be able to manage the search pages in Panels.
My initial idea was to use a search_api view and the related facetapi blocks to place them in a panel page.
That way I could work with panel variants and also export the configured panel pages.

Unfortunately the facetapi blocks seem to be rendered / prepared before the search is executed (registered in the handler SearchApiFacetapiAdapter) and because the lack of context the facet blocks don't show up.
I'm not sure if a scenario like this should be handled by facetapi or search_api - or if it's totally out of scope.

Atm. the only way I see how I could get around this is to create a ctools page manager plugin to provide a system page handler similar to the one which exists to view a node ("node/%node").
But before I start to figure out how to get this done I'd like to hear what you think about this idea :)

Has anyone an idea how to solve this with / in facetapi? I'd prefer to contribute there instead building another "wrapper".
Other concerns, suggestions, ideas?

@ #85: I already knew about this method, but I find it dubious whether it's the right way to do this. First off, I'd have to change some/most of the commit messages, as they currently aren't fit for being included as-is in the Search API commit log. Also, this would be quite a lot of commits for a single issue, which we previously didn't have and might make things messier.
You voting for that option is some help, though. If others prefer this, too, I'll just do that.

This sounds like another solution to the problem, but would have to be implemented first, so not ready for this issue yet.

@ #89: Sounds like a problem discussed (and solved) already for our current facets here: #1066278: Problems with Context/Panels and facets. May try the solution from #28 there?

@ Chris / #86:

Re #84, I am hoping to resolve the issues this weekend, however there might be a couple of outliers. I think the main issues have been addressed, and I feel comfortable rolling out a release once the filters stuff is fixed. Maybe we should target a merge by DrupalCon? Seems like a resonable timeline, and maybe we can pound out some of the remaining stuff in person during a sprint.

Originally, I hoped I could commit this before the end of GSoC, as it's now kind of part of my project. However, I guess it doesn't really matter, and maybe polishing this in a quick sprint would really be the most productive solution.
So let's just see how far we get, and if there are still substantial flaws by DrupalCon, we can just do a little sprint.

@ drunken monkey / #89: Thank you for the hint with the already solved ticket #1066278: Problems with Context/Panels and facets this did the trick. The facets show up now. However some of the facets seem to break the view when used - but I'm not sure if it's a panels / views / search_api / facetapi issue ;) I'll debug that asap. and create a patch for whatever has to be fixed.

das-peter,

Thanks for your work, testing and contributions.

@drunken monkey,

When does GSoC end? Also, what remaining issues do you think are critical to fix? I can concentrate on those. Would like to get this working sooner rather than later, as I will have a few months of coding inactivity starting in December.

Thanks,
Chris

When does GSoC end?

On August 22 (19:00 UTC, to be precise). Theoretically I'd already be in London then, but I guess this is still too soon for doing a sprint there in time.

Also, what remaining issues do you think are critical to fix?

Hm, good question.
I'll have to write some documentation and of course the migration code. For those, I'd appreciate some input in #1231570: Move this to its own module (inside the Search API project)? and #1231562: Creating/Saving facets programmatically.
Other than that, I guess we should fix #1230050: Replace legacy data key in hook_facetapi_searcher_info() (RTBC in my opinion, but thought that maybe you'd be a better judge there) and #1215526: Add support for the "Bundle" dependency plugin (which unfortunately is blocked by #1222754: Resolve assumptions in the "bundle" dependency plugin).
The remaining two issues are practically feature requests for the Facet API, which we might then adopt. I don't think it's crucial to get those in before commiting the whole work here – if someone really depends on those features, they can just keep on using our homebrew facets for a while (although I guess I'll drop them before the 1.0 release).

In any case, I doubt this will take until December. ;) I'd like to get to the 1.0 release by October (at the latest) myself.

Great timeline, and thanks for the insight. I will make sure to focus on the issues you mentioned, and feel free to add a Search API integration blocker tag to any tasks in Facet API you feel need immediate attention.

sub

Hows the integration coming along?

Almost done. Chris is currently reviewing one (maybe final) patch (#1231570-6: Move this to its own module (inside the Search API project)?), we'll probably meet up tomorrow at the sprint then and finish this thing. OK, a bit documentation (README.txt) may be missing, too, don't remember right now.

Oh, and I also have to decide how to commit this, but can maybe discuss this with Chris in person. If none of the other commiters objects, I'd go with the single-commit variant, only crediting Kat completely.

Anyways, you can already help by testing this out. Apply the attached patch, read the documentations of the new "Search facets" (search_api_facetapi) module, activate it, migrate old facets via the "Old facets" tab, and generally play around with the new facets. Same goes to all other people interested—this is almost done, time to find any remaining bugs or problems!

Ok, thanks for the reply. Do I apply this patch to any search api files? Or, where do I download contrib/search_api_facetapi ?

You apply the patch to the Search API module, and it should automatically add the files in contrib/search_api_facetapi.

Is there anyway you can provide the contents of the file, currently I've spent 3 hours trying to patch the files to find out in ssh - patch command not found. In netbeans the files did not patch at all or create new directories.

EDIT: I used ssh by installing patch and then was not able to patch any files due to it asking what files I wanted to patch... and not knowing which.

I just integrated this patch + facetapi into my search api views page and everything appears to work as expected.

I'll let you know if I find any problems.

Thanks for all the hard work. These facets make search api 100x better.

I am trying this out and like it a lot too (I had already written code for custom hierarchical facets with facet API before I switched to Search API from the Apache Solr module) - but I have a dumb question. Since the Views Integration's "Facet Block" display type was dependent on the Search API Facets module, and that no longer exists, I'm not seeing the "Facet Block" display as an option. Is there a way to replicate this functionality without the Search API Facets module?

@BeaPower - you should install git. It's free and very easy to use. Check out this page for reference: http://book.git-scm.com/. Ultimately, all you have to do is download the dev version of Search API, navigate to that directory, download the patch into it, call "git init," and then "git apply" followed by the name of the patch file.

Ok, I'll try it out, thanks. Yes, views custom facet blocks is the most important thing on my site that makes it function, its the site's main features. Please make it available if it isn't.

I am trying this out and like it a lot too (I had already written code for custom hierarchical facets with facet API before I switched to Search API from the Apache Solr module) - but I have a dumb question. Since the Views Integration's "Facet Block" display type was dependent on the Search API Facets module, and that no longer exists, I'm not seeing the "Facet Block" display as an option. Is there a way to replicate this functionality without the Search API Facets module?

Not a dumb question at all—I actually remembered I had to do this at one point, but then forgot again. I'll look into porting the block accordingly: #1261504: Remove the "Search facets" dependency for the Views "Facets block" display.

Hi Chris & Thomas,
I'm really disappointed I didn't get to meet either of you at DrupalCon - I wasn't able to make the code sprint yesterday unfortunately. And sorry for my total lack of input on this over the past several weeks - please let me know if there's any follow-up I can help with (documentation or whatever...). Also, it doesn't seem right that I would be the sole committer on this - I know you both did a ton of work on it after I pretty much handed it off.

Katherine

Kat,

Very disappointed I didn't get a change to meet you either. Unfortunately I had to leave early due to the Hurricane that is expected to hit my area, so I missed the sprint as well. Was really looking forward to coding my face of. I am totally for giving you credit, because although others did work you did most of it and really pushed this to actually happening.

Thomas,

Was great meeting you, and I enjoyed your presentation. Sorry again I had to skip out early.

Regarding this thread, I also closed an issue at #1222754: Resolve assumptions in the "bundle" dependency plugin. I will try to get the changes into Search API Facet API Integration at some point, unless someone else wants to take a crack at it.

Other than that patch is looking really good. Thanks for everyone's hard work on this.
~Chris

@ Kat: Ah, darn, you were there, too? Sorry, didn't know that, we should have definitely met up then. Would have been great to meet you—and maybe get this thing in, finally.
And I do think that you did the main work here, so I'd think it fair if you also got the main credit. And if Chris doesn't object, either, this pretty much settles it, say what you want. ;P (Also, the other contributors will be in the commit message anyways—and until some weeks ago, this was the only attribution anyone got.)

@ Chris: Yeah, I heard from Peter that you left early, really a pity. We had a small Search API sprint on Friday, and finalizing Facet API integration would have been a real high point of that. But I guess we should be able to get this done in the next few days/week in any case.

That we can use the bundle dependency now is great. Do you think we should add this to the patch, or can add this later? Or, am I seeing this right and we just need to check for a bundle key and add a value to the appropriate facet definition if one is present? Then we should of course just add it, is a minor thing. I just don't understand how a facet could be a bundle key for multiple entity types at once, or for an entity not being the one the searcher is for … Is that just a precautious flexibility, or are there really use cases and I don't fully understand this? (The patch suggests that I at least partially get this, as you apparently add 'field api bundles' => array('node'), to the „Content type“ facet.)

Also, did you already have a chance to look at #1231570: Move this to its own module (inside the Search API project)?? (Hm, but if the patch above works for you that's really the same thing, as that patch already contains the new module. So could set that to RTBC in this case, or just commit it.)
Additionally, please shortly respond in #1230050: Replace legacy data key in hook_facetapi_searcher_info(), whether we got this right (type doesn't have to be an entity type, and just changing it to a single-valued array is the way to go).

Apart from that the issue queue is looking pretty good right now, just a few (hopefully) smaller things. And the "Current search" issue, which I think we can leave for now and come back to fix when the corresponding Facet API improvements are done.

Thomas,

Glad to here there as a sprint for Search API. Definitely look forward to seeing this module mature and thrive going forward.

Regarding the dependency system, I discovered another assumption so the issue is back in review. To me it is a nice feature and solves a common design pattern, however it is by no means vital and can be resolved after the integration. Also, there is no use case in mind about why a field would index bundles for multiple entities. However, there is a use case where an index contains data for multiple entities, so it seems within the realm of possibility. Didn't want to hard code an arbitrary limitation that would block this use case from happening.

I also responded #1230050: Replace legacy data key in hook_facetapi_searcher_info(), looks good to me. Just to clarify, in most instances this will be the machine readable name of the entity. This will be represented by a single element array, i.e. array('node'). As mentioned above there is a use case for indexing data from multiple entities, in which case it could be array('node', 'comment') or something, like that.

Subscribing.
Testing patch #99.

subscribe

OK, then I guess I only have to fix #1261504: Remove the "Search facets" dependency for the Views "Facets block" display now and we'll have our complete patch candidate, great!

And we'll just move the bundle dependency plugin issue over here, once the patch has been committed.

Oh, and the explanation with multiple indexes is of course logical, hadn't thought of that.

cant wait to test this!

subscribe

Thanks to all the testers.

Just to recap, there are a few remaining issues, but they are to be resolved afterwards:
#1215526: Add support for the "Bundle" dependency plugin - Fix required ion Search API
#593658: Make the current search block more configurable - Fix required in Facet API

Also, there is a change coming to Facet API which may effect the integration. It should be a fairly minor modification, but I posted the issue below:
#1263386: Override the FacetapiAdapter::getSearchPath() method in the Search API adapter

subscribing. great work guys!

118 comments? Wow, time this gets finally fixed!

And it will be! We are finally really at „needs review“, just fixed the last blocking issue!
If you use the „Facets block“ display mode in Views, please see #1261504: Remove the "Search facets" dependency for the Views "Facets block" display for details about that. Chris, please go there in any case and see if you can answer my questions there. The thing works, but there are (at least) two places it could be a bit more beautiful. ;)

To all others: Please test!

#118 - If I apply this patch is it essentially then the same as the sandbox: Search API Facet API Integration (http://drupal.org/sandbox/cpliakas/1215178)?

do you have any snapshot files - I tried patching but nothing has worked for me!

#118, I have pulled down the latest code from the facetapi branch of http://drupal.org/sandbox/cpliakas/1215178 and it works perfect. So if that is testing your patch at #118 then all is good.

If I am missing the mark can you clarify so I can help test?

Cheers,

David

#118, I have pulled down the latest code from the facetapi branch of http://drupal.org/sandbox/cpliakas/1215178 and it works perfect. So if that is testing your patch at #118 then all is good.

The patch is the same as the sandbox, except that is also already contains #1261504-1: Remove the "Search facets" dependency for the Views "Facets block" display. If you are not using the „Facets block“ Views display mode, then it doesn't matter which one you test. The sandbox essentially contains nothing more than an updated version of #99, with the latest „normal“ Search API developments added.

However, as mentioned in the other issue, the „Facets block“ display also needs the Facet API patch in #1252644-2: Remove assumption that the facets will link back to the page they are on to work.

Finally committed the fix to #1252644: Remove assumption that the facets will link back to the page they are on. Thanks for the reminder.

I've installed this and everything appears to be working great for me. This combined with the "OR" facet makes Search API a viable solution for my faceted search needs.

Thanks for everyone's hard work on these improvements.

I just downloaded the Facetapi dev branch (assuming this patch has been merged into the dev branch of facetapi) and enabled it but I cannot seem to find how to create facets or use this module, Can anyone briefly let me know what exactly does this do if and if acheiving something like this (http://facetedsearch.davidlesieur.com/faceted_search) is possible using this with Search Lucene Api module

You have to enable the facetapi blocks on your page. Then you have to configure them.

I do not see any new block under Site Building -> Blocks, what am I missing?

@ #125: I hope you mean „Search API“? This has nothing to do with Search Lucene API.

Other than that:
- Enable the new „Search facets“ module.
- If you had previous facets, import them into the Facet API on indexes' „Old facets“ tabs.
- On the „Facets“ tab you can enable and configure the „new“ facets.
- When you habe facets enabled, the blocks will appear. (The „Current search“ blocks should be there in any case, though.

And: Did you also download the latest dev of this module and apply the patch in #118?

Hi,

I just tried the patch and after doing some testing it works just fine. Absolutely awesome work, thanks !!!

Thomas, let me know when you are going to commit so I can release a beta7 containing the patches that are currently in the dev version that facilitate Search API integration.

Unless someone complains about something else (I'll fix the first notice from #129, the second one is probably not related (I don't think this patch influences indexing at all)), I'll probably commit it on the weekend, or on Monday.
Just very busy with #1231512: Use real Relationships instead of level magic in Views integration at the moment.

Btw, thanks for testing everyone!

OK, this is it! Seemingly no more complaints, a little testing also showed no bugs.
I was daring enough to do a few changes (fix the first notice from #129, don't delete the "search_api_facets_search_ids" variable when uninstalling the old Facets module, add a search_api_facetapi.install file with that does that during uninstalling, and fix one whitespace issue) still, but I hope those are OK and won't break anything.

Anyways, committed. Yay! \o/

Thanks again very much to everyone who helped, especially of course Kat and Chris!

(Also, come to think of it, this is no "API change" per se, as we just add a new module and set another one to „deprecated“ – the base framework remains untouched.)

Excellent! I will cut a beta-7 so we have a real release that works with Search API. Great work to everyone involved. Very impressed at how quickly this happened.

~Chris

As written there, the apachesolr module does the same thing, so I'd say this is a bug in the Facet API. (Or in the core Block code which apparently doesn't pass the delta through drupal_clean_css_identifier(), weirdly enough …)

suscribing

beta 7 out yet? Is it listed in facet api?

@BeaPower

Facet API Beta 7 is finally released!

Thanks,
Chris

Hi,
I've installed the Beta 7 with search api and search api solr, everything work fine but i don't understand the differencies between Facet API and Search api facets (inside "Search api" module > contrib). Is the second module override Facet API ?
Because i can't find any menu entries defined in facetapi.module (ie : admin/config/search/facetapi/%facetapi_adapter/%facetapi_realm/%facetapi_facet/export).
Is it normal ? Do i have to disable one of theses module ? Or is FacetApi module have to be reinstalled ?
Thanks for your answer.

I've tested this hook inside my module :

 
function mymodule_facetapi_facet_info_alter(&$searcher_info) {
  dpm($searcher_info);
}

The support requests will get lost because this thread is closed. I would recommend posting another thread.

Chris