While working on #33809: Make pagers not suck I wasn't able to modify the limit property of a query using hook_query_alter() or hook_query_TAG_alter().
I tried

<?php 
$query->limit(5);
?>

as well as

<?php 
$query->extend('PagerDefault')->limit(5);
?>

The first one resulted in an error ("undefined method") and the latter didn't had any impact on the result list.

Any hints how to alter the limit? Or isn't this possible with hook_query_TAG_alter()?

Stefan

Comments

Crell’s picture

Status: Active » Fixed

The first one won't work because limit() is a method on PagerDefault only. To just limit the result set itself, use range(). See "Ranges and Limits" on: http://drupal.org/node/310075

For the second, it looks like you're probably getting bitten by #425872: DX problem with DB extenders. You're much better off extend()ing early, I think.

stBorchert’s picture

You're much better off extend()ing early, I think.

Yeah, I know.
But in this case it isn't possible. The purpose is to allow modifying Drupal's pager limits (currently hardcoded). Dries suggested using hook_query_TAG_alter() (seemed reasonable) to alter the limits.

I tried ->range(), too but unfortunately it is protected and therefore not modifiable.
So there is no chance to alter the limit value after the query has built using hook_query_TAG_alter()?

Crell’s picture

Status: Fixed » Active

Er. I'm confused. ->range() is not a protected method. The data it sets is protected, but the method is not. Can you paste your entire alter hook here and I'll have a look?

stBorchert’s picture

Hm, don't know where I saw those "this is a protected method" errors. ->range() is working (somehow).

The module code I use for testing is

<?php
// $Id$

/**
 * @file
 * Test the modification of displayed items through hook_query_TAG_alter().
 */

/**
 * Implementation of hook_help().
 */
function pagertest_help($path, $arg) {
  switch ($path) {
    case 'admin/help#pagertest':
      $output  = '<p>' . t('Test the modification of displayed items through hook_query_TAG_alter().') . '</p>';
      return $output;
  }
}

/**
 * Implementation of hook_query_TAG_alter().
 *
 * Modifies the number of displayed items for recent hits.
 *
 * @param $query
 *   An Query object describing the composite parts of a SQL query.
 * @return
 *   None.
 */
function pagertest_query_statistics_recent_hits_alter(QueryAlterableInterface $query) {
  // Set number of displayed items to 5.
  $query->extend('PagerDefault')->range(0, 5);
}

This is only valid after applying the latest patch from #33809: Make pagers not suck (otherwise there isn't any query with this tag).

The main problem with range is that the pager isn't re-calculated. It says 2 pages (~40 entries in list, default is 30 items per page) although it displays only 5 of them per page (there should be ~8 pages).

Stefan

Crell’s picture

Ah, there's your problem. ->extend() returns the new object. It does not modify it in place. So what the above code does is:

1) Take $query, and create a new PagerDefault object that references it.
2) Set that object to range(0, 5).
3) Delete that new PagerDefault object when the function ends as it has not been assigned to anything.

If $query has already been extend()ed, then you don't need to extend it again. In fact doing so will probably break things. If that still doesn't work, that's a bug in PagerDefault most likely.

If $query has not been extended, then you need to do the following:

function pagertest_query_statistics_recent_hits_alter(QueryAlterableInterface $query) {
  $query = $query->extend('PagerDefault')->range(0, 5);
}

That will replace the $query object with the PagerDefault object... which in turn still references the original $query object. Which is how the extenders work. (If that doesn't work, then there's a more subtle issue at work here.)

Does that make more sense?

Berdir’s picture

And, you don't need the extend if you just want to use range().

range(): Limit the result with *hardcoded* start - end values.
extend('PagerDefault') + limit(): Dynamic limiting, based on the page GET argument, you only define the number of elements.

stBorchert’s picture

Ok, we want to use ->limit() (otherwise its hard to use the paging functionality).
Unfortunately $query = $query->extend('PagerDefault')->limit(5); doesn't have an effect on the result list.
Maybe its to late (in the query's lifetime) to modify this value.

Berdir’s picture

Hm, I see the problem.

the alter hooks are called *inside* execute() of the normal query class. An additional problem, this (indirectly) overrides $this with a new object, and that is not allowed. So I am actually surprised that this works at all. And even if the query is already extenden (and it should be, when I understand your linked issue correctly), just calling limit() will not work, because the execute() method of PagerDefault has already been called.

The only way I currently see that would allow this is to move the logic of execute() to an internal function like "_execute()" and declare execute() as final. Query extenders would then only override "_execute" and the execute method would call the hooks first and then the (maybe overriden) _execute method.

Crell’s picture

Hm. Tricksy... I can see where we'd want to be able to extend from the alter hook, although it's not something I had originally envisioned.

If we do split execute() into two parts, then I would (a) Not make one of them final, as that is unnecessarily limiting yourself and (b) use a properly-named protected method, not the silly _foo() naming scheme that dates from PHP 4. :-)

I'm not sure that even that would work, though. Once you're inside the object, you can't change what $this is from further up the stack.

Berdir’s picture

I was just trying to to hint that it would be a protected method and I had no idea to name it properly :)

I agree, it will not work to dynamically *add* an extender, but it should be possible to change for example the limit of the already existing PagerDefault Extender. And If I understood the linked issue correctly, that's the idea.. (Under a specified condition, it should use a lower limit value for all pager queries... )

Crell’s picture

Hm. Who wants to take a crack at the patch? :-)

Berdir’s picture

Give me a name for the protected method and I'll do it ;)

stBorchert’s picture

Hi.
Did you already found a name for the new function?
What about prepare_execute(). Or pre_exceute()?

Berdir’s picture

stBorchert’s picture

Oh, thanks.
I wasn't aware of that issue (though I knew its blocker #481288: Add support for INSERT INTO ... SELECT FROM ...).

stBorchert’s picture

Status: Active » Fixed

#496516: Move query_alter() into a preExecute() method is in so we can do it very simple by using code like this:

<?php
/**
* Implement hook_query_alter().
*/
function mymodule_query_alter(QueryAlterableInterface $query) {
  // Set number of displayed rows to 10 for recent hits page.
  if ($query->hasTag('statistics_recent_hits')) {
    $query->limit(10);
  }
}
  
/**
* Implement hook_query_TAG_alter().
*
* Alter the number of displayed rows on dblog/overview.
*/
function mymodule_query_dblog_overview_alter(QueryAlterableInterface $query) {
  // Set number of displayed rows to 30.
  $query->limit(30);
}
?>

If the query has a tag you could easy modify its limit (and all the other attributes).

Status: Fixed » Closed (fixed)

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