Needs review
Project:
Apache Solr Views
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
30 May 2014 at 12:35 UTC
Updated:
13 Dec 2015 at 16:42 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
nilanjanroy commentedBelow patch file will fix the problem as above to expose the offset value as start index
so that from
build_info can provide
the offset value and we can easily inject this value in a start param of apachesolr
Comment #2
nilanjanroy commentedComment #3
miroslavbanov commentedThere is a problem with the offset being ignored. This patch doesn't resolve it.
hook_apachesolr_query_alter($query)can't have access to some property that is set to $view.hook_views_query_alter($view, $query)is called before apachesolr_views_query::build(), so that's not it either.I think this should be resolved in apachesolr_views module directly, without relying on some alter hook.
Comment #4
miroslavbanov commentedApparently,
apachesolr_do_query()replaces the 'start' param. Here is a patch that correctly gets the pager offset, and uses the ApachesolrViewsSolrBaseQuery::search() method instead, which doesn't replace it.This prevents the call to hook_apachesolr_query_prepare(), hook_apachesolr_query_alter(), query static response caching, facetapi integration, and HTML entity-encoding of query parameters.
Comment #5
roynilanjan commentedOK seems to me it should work, the actual skipping is coming from apachesolr_do_query which has replaced by search() method.
My apparoach was to expose an extra element with build_info(along with the offset value which is set in view) and using hook_apachesolr_views_alter() i can easily the access start param.
According to apache_do_query() there are two methods, prepare_query(which is invoked before start param manipulation) & alter_query(after start param manipulation)
Comment #6
miroslavbanov commentedMy previous patch was not working correctly for 'full pager', when you start navigating to the next pages, the pages count decreased, or the pager disappeared. Here is a new patch that is better.
I don't think we should re-implement apachesolr_do_query(). The hooks that it provides are not needed (views has plenty already), and the caching actually prevents two apachesolr views from working correctly on the same page. The only good feature is the facetapi integration, but it is quirky and doesn't work in combination with views exposed filters, and it completely relies on the aforementioned caching.
It is possible to have some setting that would switch between using facetapi integration, and not using it, but I don't think it is very valuable while it is not working correctly in combination with views exposed filters/sort.
Comment #7
roynilanjan commentedpatch in #6 works as expected!!!
Comment #8
roynilanjan commentedSome more review needs as I tested over my application seems it breaks, as this patch internally escapes the
. So if i have query_prepare in module , then those logic will not be triggered at-all as prepare query call from it only!
Comment #9
miroslavbanov commentedAs I explained in #4, apachesolr_do_query() does many things, which will no longer be done after applying the patch. This is pretty much a backwards-incompatible change.
One of these things apachesolr_do_query() does, is reset the 'start' parameter, which prevents the pager offset form working - a core views functionality. That's why I removed it. Also, the query caching in apachesolr_do_query(), is what prevents two apachesolr views, on the same page, from working correctly. apachesolr_do_query() looks like a function that was designed to be used from apachesolr_search, and not suited for all apachesolr modules. Also, the hooks that are called there are superfluous, as views already provide many hooks that can be used.
I agree that we need more testing. I was thinking about changing the status to "needs review" myself. Such a change may be done in a new branch/major release as well. I'm open to ideas and patches.
Comment #10
miroslavbanov commentedPrevious patch no longer applies.
Comment #11
ronaldmulero commented#10 works for me, but without the "$params['rows'] = $this->limit;" modification.
Also, might want to add a check for offset:
Comment #12
aron novakIndeed isset() is needed. For the limit propery, I left it like that, maybe @MiroslavBanov can comment on the purpose, for now the patch seems to be fine, ready for further tests and reviews.
Comment #13
miroslavbanov commented@Aron Novak
I can't remember exactly what I had in mind. It worked for me, and it is more in line with
views_plugin_query_default.My test scenario was a embedding "Views pane" ctools content into Panel page. The ctools content is in the "views_content" module that is a sub-module of "ctools". I am very much interested in understanding why exactly the other cases are needed, and what they solve as a test scenario.
Comment #14
aron novak@MiroslavBanov do you have chance to test #12? Then we could see if it broke your scenario or not.
Comment #15
miroslavbanov commentedCan't test the scenario right now. I am going to go ahead and assume it does work with the isset(). I am still interested in under what circumstances it doesn't work without isset().
Comment #16
aron novakMy scenario is a simple ApacheSolr Views-based view (7.x-1.1-beta1) , Views version (7.x-3.11). infinite scroll pager (with patch views_infinite_scroll-views_responsive_grid-1977012-8563875.patch), the view itself was a Page with its own path.
Comment #17
miroslavbanov commentedI tried the setup you described, with the patch you use, and these versions:
I am still seeing the isset() is superfluous.
I see that the call to
sets offset and limit at the end: