Comments

nilanjanroy’s picture

Below patch file will fix the problem as above to expose the offset value as start index
so that from

hook_apachesolr_query_alter($query)

build_info can provide
the offset value and we can easily inject this value in a start param of apachesolr

nilanjanroy’s picture

Issue summary: View changes
miroslavbanov’s picture

Status: Needs review » Needs work

There 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.

miroslavbanov’s picture

Apparently, 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.

roynilanjan’s picture

OK 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)

miroslavbanov’s picture

Title: Pager offset value is always resetting 0 even view display have specify offset » Pager offset value is always resetting 0
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new1.52 KB

My 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.

roynilanjan’s picture

Status: Needs review » Reviewed & tested by the community

patch in #6 works as expected!!!

roynilanjan’s picture

Status: Reviewed & tested by the community » Needs review

Some more review needs as I tested over my application seems it breaks, as this patch internally escapes the

apachesolr_do_query()(using search())

. So if i have query_prepare in module , then those logic will not be triggered at-all as prepare query call from it only!

miroslavbanov’s picture

As 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.

miroslavbanov’s picture

Previous patch no longer applies.

ronaldmulero’s picture

Issue summary: View changes

#10 works for me, but without the "$params['rows'] = $this->limit;" modification.

Also, might want to add a check for offset:

    if (isset($this->offset)) {
      $params['start'] = $this->offset;
    }
aron novak’s picture

Indeed 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.

miroslavbanov’s picture

@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.

aron novak’s picture

@MiroslavBanov do you have chance to test #12? Then we could see if it broke your scenario or not.

miroslavbanov’s picture

Can'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().

aron novak’s picture

My 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.

miroslavbanov’s picture

I tried the setup you described, with the patch you use, and these versions:

$ drush pml --status=enabled --type=module | grep "apachesolr\|views"
 Search Toolkit   Apache Solr framework (apachesolr)                  7.x-1.8
 Search Toolkit   Apache Solr search (apachesolr_search)              7.x-1.8
 Search Toolkit   Apache Solr Views integration (apachesolr_views)
 Views            Views (views)                                       7.x-3.13
 Views            Views Infinite Scroll (views_infinite_scroll)       7.x-1.1
 Views            Views Responsive Grid (views_responsive_grid)       7.x-1.3
 Views            Views UI (views_ui)                                 7.x-3.13

I am still seeing the isset() is superfluous.

I see that the call to

    // Let the pager modify the query to add limits.
    $this->pager->query();

sets offset and limit at the end:

    $this->view->query->set_limit($limit);
    $this->view->query->set_offset($offset);