If limit parameter in EntityFieldQuery::pager()
is zero, it leads to division by zero in pager_default_initialize
. It is in contradiction with method documentation, which reads that zero should disable pager
Scenario looks llike the following:
EntityFieldQuery::pager() creates $this->pager = array('limit' => 0 ...
probably, this is incorrect if limit is zero
EntityFieldQuery::initializePager() checks if ($this->pager && !$this->count)
and accepts it
EntityFieldQuery::initializePager() invokes pager_default_initialize(..., $this->pager['limit'], ...);
pager_default_initialize executes ceil($pager_total_items[$element] / $limit)
and gets division by zero
Comment | File | Size | Author |
---|---|---|---|
#23 | d7-entity-pager-1490150-23.patch | 2.04 KB | xjm |
#20 | d8-entity-pager-1490150-20.patch | 923 bytes | xjm |
#17 | d8-entity-pager-1490150-14.patch | 923 bytes | xjm |
#17 | d7-entity-pager-1490150-14-do-not-test.patch | 2.04 KB | xjm |
#12 | drupal-1490150-12-tests.patch | 1.15 KB | tim.plunkett |
Comments
Comment #1
tms320c CreditAttribution: tms320c commentedComment #2
catchYes there's no check for this at all. Patch will need to be filed against 8.x and backported to 7.x, we should also add an automated test for this.
Comment #3
duellj CreditAttribution: duellj commentedHere's a patch for 8.x. Adds a test that enables a short pager, then disables it. Once this gets approved I can create a backport to 7.x
Comment #4
duellj CreditAttribution: duellj commentedUpdated the docblock for EntityFieldQuery::initializePager(), since it doesn't reflect the fact that you can disable the pager by setting the pager limit to 0.
Comment #5
catchUploading just the test change so we can see it fail.
Comment #7
dawehnerIs there a specific reason why there is two calls to pager()?
Comment #8
duellj CreditAttribution: duellj commented@dawehner: The first call sets the pager to show one item, and the second call disables the previously set pager (to test that disabling a previously set pager with ->pager(0) works properly).
Comment #9
aspilicious CreditAttribution: aspilicious commentedStill needs review for the patch in #4.
Comment #10
catch#4 looks RTBC to me, patch is simple, test is good.
Comment #11
catchCommitted/pushed to 8.x, moving to 7.x for backport. Also apologies, I copy pasted the dreditor message without thinking and gave myself commit credit for uploading the 'test only' patch :(
Comment #12
tim.plunkettThe diffstat is off by a line because I didn't include the extra blank line that was in the above patch.
Comment #13
xjm"Tests the disabling the pager"?
Comment #17
xjmSomething strange going on with the upload. Edit: finally.
Comment #18
tim.plunkettShouldn't this say "is enabled and then disabled"?
Comment #19
xjmNo?
Edit: Yes.
Comment #20
xjmComment #21
duellj CreditAttribution: duellj commentedAll text changes look good and proper. Thanks xjm!
Comment #22
webchickCommitted and pushed that patch to 8.x. Back to 7.x for the backport, which presumably needs a re-roll to incorporate those fixes.
Comment #23
xjmDidn't I... I guess I didn't.
Here it is.
Comment #24
tim.plunkettLooking good!
Comment #25
webchickCommitted and pushed to 7.x. Thanks!