Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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
Comment #1
Crell CreditAttribution: Crell commentedThe 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.
Comment #2
stBorchertYeah, 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()
?Comment #3
Crell CreditAttribution: Crell commentedEr. 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?
Comment #4
stBorchertHm, 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
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
Comment #5
Crell CreditAttribution: Crell commentedAh, 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:
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?
Comment #6
BerdirAnd, 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.
Comment #7
stBorchertOk, 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.
Comment #8
BerdirHm, 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.
Comment #9
Crell CreditAttribution: Crell commentedHm. 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.
Comment #10
BerdirI 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... )
Comment #11
Crell CreditAttribution: Crell commentedHm. Who wants to take a crack at the patch? :-)
Comment #12
BerdirGive me a name for the protected method and I'll do it ;)
Comment #13
stBorchertHi.
Did you already found a name for the new function?
What about
prepare_execute()
. Orpre_exceute()
?Comment #14
BerdirSee #496516: Move query_alter() into a preExecute() method
Comment #15
stBorchertOh, thanks.
I wasn't aware of that issue (though I knew its blocker #481288: Add support for INSERT INTO ... SELECT FROM ...).
Comment #16
stBorchert#496516: Move query_alter() into a preExecute() method is in so we can do it very simple by using code like this:
If the query has a tag you could easy modify its limit (and all the other attributes).