According to the views documentation:

The flag $view->get_total_rows is used to force the query of the view to calculate the total number of results of the set.

This documentation further notes that the following hook should be used to manually force the count to be calculated if the pager settings do not trigger the calculation automatically:

function my_module_views_pre_execute(&$view) {
  if ($view->name == 'my_view' && $view->current_display == 'my_display') {
    $view->get_total_rows = TRUE;
  }
}

As of 7.x-1.10 this trick will no longer work for views based on Search API data. I did some hunting and I believe the commit that broke things was the fix for #2135363: Add support for Views' use_count_query() method. With that fix in place $view->pager->use_count_query() is the only variable checked when deciding if the result count should be calculated, the value of $view->get_total_rows is not incorporated at all.

In fact from what I can tell, before #2135363 the result count was always calculated no matter what, so the value of $view->get_total_rows was irrelevant (and the result count was always available).

Anyway, now that Search API is being smart about deciding when to calculate a result count, I figure it should be a bit smarter and also utilize the $view->get_total_rows variable as recommend by views.

Marking as a bug because people who have custom logic that depends on the result count value being available will have broken views after installing 7.x-1.10 (no matter what they do to try and force it to be calculated). This may-or-may-not be the appropriate classification though.

CommentFileSizeAuthor
#4 2155721-4--views_get_total_rows.patch1.63 KBdrunken monkey
#2 2155721-2--views_get_total_rows.patch1.62 KBdrunken monkey
#1 search_api-views_allow_forced_calc_of_total_result_count-2155721-1.patch2.1 KBrjacobs

Comments

rjacobs’s picture

rjacobs commented
Title: Views - No way to force calculation of total results count ($view->get_total_rows) » Views - No way to force calculation of total results count (e.g. check value of $view->get_total_rows)
Status: Active » Needs review
StatusFileSize
new search_api-views_allow_forced_calc_of_total_result_count-2155721-1.patch2.1 KB

Ok, here's a patch, made with git format-patch, which seems to address this. It's fairly innocuous and just ensures that the value of $view->get_total_rows is checked whenever $view->pager->use_count_query() is checked. This happens in both the query build() and execute() methods, as that seems to be the plan after issue #2135363 was addressed. However, I'm not totally sure why this check needs to be in the build() method given that I think any service methods that would care about it would not be triggered until the execute() method.

Anyway, there it is... I hope someone can have a look.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Title: Views - No way to force calculation of total results count (e.g. check value of $view->get_total_rows) » Add support for Views' get_total_rows property
StatusFileSize
new 2155721-2--views_get_total_rows.patch1.62 KB

God, I hate Views … Can't anything not have eight million undocumented extensions?!?</mandatory_rant>
Anyways, thanks for reporting this issue and even providing a patch to solve it! Without people like you who understand at least part of this monster, I'd be completely lost. ;)

The patch didn't apply cleanly for me, attached is a re-roll for current HEAD. I also removed the first setting of the skip result count option – as you say, that shouldn't really be necessary.

I've also added the feature that other handlers can just set the skip result count themselves to either value and it won't be overridden – so you could set that to TRUE, if you are really sure, even if the pager or other components need the result count. Of course, it shouldn't be normally used.
Do you think this makes sense? Or should we just check for a FALSE value to let handlers express their need for a result count this way, too?

Anyways, thanks again for the explanation and patch! And sorry I didn't check before committing the previous issue, I just trusted drumm to know what he's doing (which, in the case of Views, probably no-one ever really does).

rjacobs’s picture

rjacobs commented

Yeah, views is quite the animal given all the use cases it ends up catering to.

You suggestions make sense, and it's good to see that the check related to the "skip result count" option can be safely postponed to the execute() method. I can confirm that your version of the patch also works in our test case using search_api_db as well as a separate test case using search_api_solr.

You also mentioned changing things so that "skip results count" can be set by other handlers and then not overwritten. I probably don't know enough about the various cases where handlers would be doing this to be too much help, but I suppose it could raise a problem case where a "higher level" views method tries to set $view->get_total_rows but this setting is still not honored because a search api handler has calculated internally that skipping the count is ok. I guess it's a question of which property should get priority, $view->get_total_rows or the search api query "skip result count" option (as set before the query is executed). I feel a bit like $view->get_total_rows should be able to trump everything... but I'd trust your personal judgement the most on that one.

No matter what, the work that you guys did to ensure count queries are conditional is far more important than catering to all these special cases. So getting that in there and fleshing out these anomalies later seems pretty reasonable.

I'm curious why my patch didn't apply cleanly for you. I'm pretty sure it was against the most recent HEAD. I wonder if it's got something to do with using git format-patch? I've gotten in the habit of using that to make attribution easier (I'm always trying to boost my d.o. profile street cred.), but if that gets in the way I'll have to switch back to just git diff.

Thanks for the quick attention with all this!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2155721-4--views_get_total_rows.patch1.63 KB

You also mentioned changing things so that "skip results count" can be set by other handlers and then not overwritten. I probably don't know enough about the various cases where handlers would be doing this to be too much help, but I suppose it could raise a problem case where a "higher level" views method tries to set $view-&gt;get_total_rows but this setting is still not honored because a search api handler has calculated internally that skipping the count is ok. I guess it's a question of which property should get priority, $view-&gt;get_total_rows or the search api query "skip result count" option (as set before the query is executed). I feel a bit like $view-&gt;get_total_rows should be able to trump everything... but I'd trust your personal judgement the most on that one.

I guess you're right, letting one "trump" the others is a recipe for confusion. People will still be able to override this in the Search API preprocessing chain, if they are really sure they want to, so why make the base code more complicated than necessary?
The attached patch changes it to make all three ways of specifying a required result count equal. If no-one objects in the next few days, I'll commit it.

I'm curious why my patch didn't apply cleanly for you. I'm pretty sure it was against the most recent HEAD. I wonder if it's got something to do with using git format-patch? I've gotten in the habit of using that to make attribution easier (I'm always trying to boost my d.o. profile street cred.), but if that gets in the way I'll have to switch back to just git diff.

Sorry, I can't tell you either. Maybe I committed some patch in between your post and my testing the patch? In general, git format-patch should work just as well as git diff, I think. However, if I'm not mistaken it also preserves the patch's history (i.e., commits made), which can get more complicated to review and might also introduce some complications if you merge while creating the patch. I'm not sure, though, I always use git diff myself.
Also, I always aim to give proper credits to patch creators, so no worries there. I find it annoying that other don't, too, though. But I don't think using git format-patch really changes that.

rjacobs’s picture

rjacobs commented

Cool, this looks good. I see that you assert a default value of TRUE for the 'skip result count' if it's not set and then only test it in the execute() method if it is TRUE. That seems reasonable as the $view->get_total_rows value will always be tested except for those special cases where its already known internally that the count must be calculated.

This checks out again with tests that use search_api_db and search_api_solr.

Thanks also for the notes about the patch. No matter what git format-patch certainly introduces more metadata that may be problematic in certain situations (on the receiving end), so I may use it a bit more sparingly.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Good to hear. Thanks again for your work, and for helping me finalize the patch!
Committed.

rjacobs’s picture

rjacobs commented

Thanks! Also thanks for the commit attribution, etc.

idebr’s picture

idebr commented

This also fixes an issue with Views where the 'More'-link is not displayed in a block display with the option 'Only show when more content is available' enabled

Status: Fixed » Closed (fixed)

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