Other GET parameters ignored by Apache Solr Facet Blocks

We worked hard to remove all references to $_GET from the class. Seems like it would make more sense to pass in $_GET to the constructor.

Also, drupal_query_string_encode() has a serious bug in that it does:

$key = drupal_urlencode($key);

I'll have to find the issue link.

I don't love the class taking in $_GET... I suppose it is okay, but for DX, classes that take anonymous arrays of "stuff" are usually deadly. Seems to me that parsing $_GET should be outside of the class, and perhaps making sense of some random variables could be in a class factory. But the constructor should, if possible, be clean of that kind of logic and ambiguity.

If we just shove $_GET in as a property of the class along with other well defined ones in the constructor, I'm okay with that, as long as the $_GET munging doesn't happen there.

It's not clear to me whether we need to worry about it, but maybe we shoudl based on this function: http://api.drupal.org/api/function/filter_xss_bad_protocol/6

Actually, if we are doing it we shoudl probably do it when we output the querystring, rather than on input?

@Robert - we are using rawurlencode() now which shoudl encode spaces as %20, not +.

Filter on output is the Drupal way - I think we shoudl stick to that pattern.

@Robert - why would we use a static class variable for this? Seems to defealt the purpose of allowing overrides.

this patch makes changes that should also be reflected in the Interface.

I also don't understand the need for this change. Scares me a lot, I really like the class to know nothing but the stuff that it did makes it easy to test, easy to understand. What is the intended bug to be fixed or feature to be added?

Here's a start on a patch that's a more general cleanup. It's totally untested, but just so you can see what I'm thinking.

@Scott - I think the idea is that there might be extra GET params you care about, especially for custom search pages. I'm not sure what Robert's particualr motivation is in this case.

Ok thanks for the explanation. Changing title to reflect the problem better and providing a demonstration patch. Unfort, I am stuck on another project, which has a solr version that is lower and I really don't have time to spend. But as this affects the Views implementation, and I feel pretty strongly about this patch, I created a mere demonstration patch. It does have bugs in it, namely it will repeat variables in some cases. But I believe the code is placed in the proper spot so it affects how we create links and doesn't affect how we create Query objects.

Probably doesn't apply to the latest dev, again don't have time, brain cycles to spare.

Works in ur example though just as you drive deeper repeats GETs.

Tested against Apache Solr Views.

Sure, we could roll something liek that - but I'm not sure how this avoid duplicating params like solrsort, and of replicating aprams you don't want like 'page'
$diff = array_diff_assoc($_GET, $options);

We should be able to use this helper function I think
http://www.php.net/manual/en/function.http-build-query.php

@Scott - the downside of doing it the way you suggest, is that there is no easy OO way to override the behavior. Not sure what the right trade-off is.

Actually - thinking more about it - this also would potentially alleviate the $_GET hack I just committed in the taxonomy browsing pages.

is that there is no easy OO way to override the behavior

this is the point. why would the query object every want to override the behaviour. What would it do differently about things that it knows nothing about?

And as Robert stated in the opening of the ticket

The facet links don't have any awareness of other GET parameters that might be in the URL other than the ones that AS puts there itself

Its the facet links, not the object that should concern itself here.

This is more like what Scott suggests - but I see again that it's problematic in as much as we will fail to retain any GET params when we submit the search form.

@Scott - the query object is providing you with a querystring (or possibly an array of query values). How it generates that does seem to be something you might want to override - e.g. if you want your url to look like:

?facets=tid:99&mysort=created desc

or like project module does, to add keys to a 'text' GET param

This patch fixes the sort link for score and the retain filters behavior - though note that misc GET params are not retained except when we specify retain-filters.

I think this might be a bit of a flaw in this architecture.

the query object is providing you with a querystring (or possibly an array of query values). How it generates that does seem to be something you might want to override

Right but those are parameters it knows about. things like xor or print it doesn't know about. By making the class retain these GETs we have the class maintain things it doesn't know how to handle and thats a lil scary. The class itself should be a self contained object. How do ever generate a link that doesn't have those GETs in it now. At least with my approach you could use the object to generate the link.

Which is why I was searching for real tangible reason why this needs to happen. So we couldn't just say "Look project module does it" or look "Solr Views does it as well". An tangible example of where the current architecture fails, where one module fails to function in the optimum because this change isn't present. The Views object adds the exposed filters' identifiers to the query string, those can be arbitrary.

we will fail to retain any GET params when we submit the search form.

Doh! probably need to add the additional GET params as hidden values in the form. Same problem would occur in the Views object.

But in the end, I can make the Views object handle this, it already stores all the information it needs. It is small and trivial I guess.

@Robert - sure, they were jsut touching some of the same code.

After thinking abotu this more I've wavering back towards Scott's approach.

To do that well, however, I think we want to return an array of query params so that we can more easily filter by keys.

I think that if you flip the array_merge() you solve your problem

array_merge($options['query'], $_GET);

I know for a fact that this won't work with Solr Views because it doesn't use $_GET['filters']. So with the array_merge flip described above, that should work with Solr Views, and I think fix the need to unset filters.

I applied the patch and try to test on a site Im building, and got a preg_match() error from drupal_validate_utf8 when rendering the hidden apachesolr_search_querystring form element. (Taking a guess at the form element, by looking at the backtrace).

Robert - why unset when use can use array_diff_key()?

Also - for example - when we are handling a form submission we don't want to carry over any filter param from $_GET since there may be no such key in the destination query values.

ussing array_diff_key() and retaining GET params on form submission too.

I read the patch, and although I haven't been involved in the issue, I think I get the gist. Turning the query into an array instead of a string seems to have not introduced any regressions.

Would be nice to see it go through our QA cycle, but AFAICT, no issues. I am not testing it with Views or anything like that.

-J

Feature is incomplete. See #511372: Other GET parameters ignored by Apache Solr breadcrumb

Thought I would point out as you guys rev up to end the 1.x branch, that this fix does not work with Solr Views. #508826: Currently Broken with latest apachesolr dev

And the reason is

$get = array_diff_key($_GET, array('q' => 1, 'page' => 1, 'filters' => 1, 'solrsort' => 1, 'retain-filters' => 1));

That happens to be very hardcoded for apachesolr_search.module.

I pointed this out here:
1.) http://drupal.org/node/502976#comment-1764658
2.) http://drupal.org/node/502976#comment-1761590

I will fix this, but I busy busy busy. Hopefully, tonight before I pass out.

@Scott - sorry if I misunderstood your prior comments - you seemed to be arguing to not include this query parameter handling in the object.

Where do you suggest defining this filter list if it's going to be variable?

This is now usable by Apache Solr Views. It makes no assumptions about what $_GET params are in the url

For instance, Apache Solr Views might have the search keys as $_GET['my_text'] and have node types exposed via a form like this $_GET['node_types']. And this will work by leveraging the Interface and the abstraction it provides

I swear I look at the patch through less... sigh. Reroll

I have fixed Apache Solr Views to now work :-D. This patch carries the params through with standard solr and the Views query.

....

the patch is just supposed to change one line

-  $get = array_diff_key($_GET, array('q' => 1, 'page' => 1, 'filters' => 1, 'solrsort' => 1, 'retain-filters' => 1));
+  $get = array_diff_key($_GET, array('q' => 1, 'page' => 1), $options['query'], apachesolr_current_query()->get_url_queryvalues());

This would be why I stop committing code after 10pm. Brain refuses to function....

Bumping this as this functionality currently borken with Apache Solr Views. Just a one line change.