As per the post at http://drupal.org/node/384952#comment-4481748, the API should be changes so that facets define a set of query types as opposed to a single one. This will require that the definition be changed to "query types" and accept an array of types. This will need to be coordinated with Apache Solr.

CommentFileSizeAuthor
#25 1161434-25.patch18.66 KBnick_vh
#21 facetapi-1161434-21.patch18.71 KBnick_vh
#20 facetapi-1161434-20.patch21.88 KBnick_vh
#19 facetapi-1161434-19.patch18.55 KBcpliakas
#16 facetapi-1161434-16.patch18.78 KBcpliakas
#14 1161434.patch11.68 KBnick_vh
#13 1161434.patch11.21 KBnick_vh
#10 1161434.patch11.98 KBnick_vh
#8 1161434-queryType.patch11.1 KBnick_vh
#7 queryType-1161434.patch8.13 KBnick_vh

Comments

cpliakas’s picture

cpliakas commented
Status: Active » Postponed

Postponing, because this needs to be vetted out further.

cpliakas’s picture

cpliakas commented
Assigned: Unassigned » cpliakas
Priority: Normal » Major
Status: Postponed » Active

This task is important and related to #1176814: Re-implement the widget requirements system. Assigning to myself since development of this issues are so intertwined and critical to the low-level architecture of the module.

cpliakas’s picture

cpliakas commented
Issue tags: +7.x-1.0-beta4 blocker

Marking as a beta4 blocker.

cpliakas’s picture

cpliakas commented
Issue tags: +Backwards compatible API change

Adding new tag to track backwards compatible API changes. Legacy code should be removed when the module goes 1.0.

cpliakas’s picture

cpliakas commented
Issue tags: -7.x-1.0-beta4 blocker

Changed my mind. There are more important fixes that should be released ASAP.

cpliakas’s picture

cpliakas commented
Issue tags: -Backwards compatible API change +D7 stable release blocker

This is a stable release blocker that prevents the fieldset realm from being implemented. In other words, this prevents the creation of widgets that depend on different query types.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new queryType-1161434.patch8.13 KB

An initial stab to the problem
It took me a long time because figuring out the architecture just by debugging it wasn't easy

What is allows you is to install a widget and the widget can tell facetApi for what queryTypes it is working.
An example can be seen with facet_slider and adding the patch #1316268: Query Type Example restriction (follow up on apachesolr and facetapi)

There is a known (severe) bug. It will allow you to select the widget but it will not show up because I'm too tired to keep debugging for the evening :-)
I guess that someone familiar with the api will see the problem. As far as I know the problem lies in the widget. (http://drupal.org/sandbox/cpliakas/1160920)

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 1161434-queryType.patch11.1 KB

Some bugfixes - everything is working in regards to FacetApi.

I'd like a brainstorm session in regards to the use of realm in most of the functions. This queryType selection will only work when the module that is retrieving the filters does something like this 

$adapter->addActiveFilters($current_query, 'block');

As you can see the module (in this case apachesolr) tells facetapi which realm it wants. If it doesn't do that it will return the facets as defined by default and could in some cases return unwanted results.
I don't think there are any other side-effects for other modules (searchapi) by applying this patch since I tried to work towards backward compatibility and sensible defaults.

cpliakas’s picture

cpliakas commented
Status: Active » Needs work

Hi Nick.

Thanks for taking a crack at this difficult problem. My comments are listed below:

  • The FacetapiAcapter:processFacets() method is meant to process facets once per page load. This gets the data from the server an initializes the render array. Therefore it doesn't make sense to pass a realm name, since effectively it could change depending on which realm is rendered first.
  • I am looking to change the key to "'query types" (with an s at the end) to accurately reflect there are multiple types that can be defined. There are some other instances where this has been done, so there has to be some logic converting the old "query type" to "query types" for backwards compatibility.
  • There are some changes to "assignment in operator" logic that are not related to the functionality of this patch. Let's stay away from modifying other code if not necessary.

In terms of our conversations on the side, we may have to resort to adding the query type as a global setting.

Thanks,
Chris

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 1161434.patch11.98 KB

Great comments,

I tried to cleanup the patch as good as possible so it only reflects the changes that have to be made
The key has been changed to query types in the facet defintion. However, in the widget it stays query type

Widget definition : 

'facetapi_slider' => array(
      'handler' => array(
        'label' => t('Slider'),
        'class' => 'FacetapiWidgetSlider',
        'query type' => 'numeric_range',
        'requirements' => array(
          'facetapi_requirement_facet_property' => array(
            'query types' => 'numeric_range',
          ),
        ),
      ),
    ),

The numeric_range requirement could disappear, but I found it to be flexible enough so more reviews on that are welcome

Also the query type is now global, so we should still add some notification if there are multiple realms and conflicting widgets...

cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review

Great work. This definitely looks better. I will try to apply the patch and test it out.

Thanks for your hard work on this,
Chris

cpliakas’s picture

cpliakas commented
Component: Code » Code (functionality)
Status: Needs review » Needs work

I am marking this as needs work for the reason that the addActiveFilters() and process() methods are realm agnostic and only run once. If we make these realm specific then we are are missing out on some data. For example, let's say the "bundle" facet is active in the fieldset realm, but not in the block realm. If the block realm is processed first, then since those functions are only processed once the "bundle" facet won't have the data it needs in the fieldset realm. If you take a look at the application flow in the comment at http://drupal.org/node/1302590#comment-5091284, nothing in the one time processing list should be tied to a realm. Only tasks related to the widget should have realm parameters or context.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 1161434.patch11.21 KB

Updated with some changes towards your comments.
The only realm reference I left is in

public function buildRealm($realm_name) {
nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs review » Needs work
StatusFileSize
new 1161434.patch11.68 KB

Yeah a better update!
A big bug was fixed in this patch, namely that the facets disappeared whenever they were selected. This version also takes into account that if the build from the facet didn't return a value it will render the empty_behavior plugins.

Please try it out with the facet_slider and apachesolr module (see links to the respectively module and issue page in the comments above)

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs work » Needs review
cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review
StatusFileSize
new facetapi-1161434-16.patch18.78 KB

I started the query-types for work on this. The attached patch takes some of the concepts in the patch above but reworks the logic a bit to handle this in a slightly different way. Also adds a validation handler so people can't configure settings that don't make sense.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

I like the getSettings change, it makes it less obfuscated and at first sight you added more export options? A list of what you've done would be useful.

cpliakas’s picture

cpliakas commented

Sure thing. The bullet list is below:

  • In facet definitions, renamed "query type" key to "query types" and changed data type to an array to accept multiple values.
  • Added code in facetapi_get_facet_info() function to make the API change backwards compatible.
  • Added "query types" key to widget definitions flagging which query types the widget supports.
  • If a facet has multiple query types, added global setting so user has to choose which one to use.
  • Added a validation handler to make sure the selected widget supports the selected query type.
  • In the Adapter's constructor, load the query type plugins based on the global settings (if the facet supports more than one query type).
  • Moved FacetapiAdapter::getSettings() method to the adapter, separated it into two methods for global and realm specific settings.

Regarding moving the FacetapiAdapter::getSettings() method to the adapter, I had to do this since the settings now need to be loaded while the adapter is being instantiated. I took the opportunity to break the method into two settings in order to eliminate a big if / else block that was ugly looking and hard to follow at first glance. The functionality is the same, it was just moved and refactored a bit.

cpliakas’s picture

cpliakas commented
StatusFileSize
new facetapi-1161434-19.patch18.55 KB

Revised patch removing some extra, unnecessary code in the adapter's constructor.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new facetapi-1161434-20.patch21.88 KB

It wasn't working as I expected with the facet_slider

There were some mistakes in regards to the retrieval of the query_type but should be fixed now. Using this patch in a demo tomorrow so I'm marking it as reviewed in the hope that it gets committed ;-)

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new facetapi-1161434-21.patch18.71 KB

Patched against wrong version. Correct patch now

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

So for me this is ready to go! I already modified facet_slider to work with these recent changes

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work

Your differences are good. The two things I would change are building the $query_types variable via drupal_map_assoc() and still build the #options with array_map('check_plain', $query_types) to be über-paranoid about security. Other than that this is RTBC.

cpliakas’s picture

cpliakas commented
Assigned: cpliakas » Unassigned

Removing myself from the "Assigned" tag since Nick is doing most of the work here.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 1161434-25.patch18.66 KB

Found some other major bugs in regards of passing a facet Array instead of an facet object.
Also the drupal_map_assoc should be solved

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Seems like this breaks OR facets. Still have to confirm if it is this patch that breaks it or the apachesolr patch

cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review

Excellent! Thanks for rolling this. Will test and marking as "needs review".

cpliakas’s picture

cpliakas commented
Status: Needs review » Reviewed & tested by the community

I tested the OR facets, and everything works fine for me. Great work, marking this as RTBC.

Thanks, Nick!
~Chris

cpliakas’s picture

cpliakas commented
Status: Reviewed & tested by the community » Fixed

Resolved in commit fb1d0b5.

Congrats on becoming the 9th committer to Facet API!

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Cool! Great news to see this committed

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