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

CommentFileSizeAuthor
#23 d7-entity-pager-1490150-23.patch2.04 KBxjm
#20 d8-entity-pager-1490150-20.patch923 bytesxjm
#17 d8-entity-pager-1490150-14.patch923 bytesxjm
#17 d7-entity-pager-1490150-14-do-not-test.patch2.04 KBxjm
#12 drupal-1490150-12-tests.patch1.15 KBtim.plunkett
#12 drupal-1490150-12-combined.patch2.01 KBtim.plunkett
#5 entity-disable_pager-1490150-4.patch1.33 KBcatch
#4 entity-disable_pager-1490150-4.patch2.25 KBduellj
#3 entity-disable_pager-1490150-3.patch1.89 KBduellj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tms320c’s picture

tms320c CreditAttribution: tms320c commented
Title: EntityFieldQuery::pager(0) generates 'divide by zero' » EntityFieldQuery::pager(0) generates PHP error 'divide by zero'
Priority: Normal » Major
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 7.12 » 8.x-dev
Issue tags: +Needs tests, +Needs backport to D7

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

duellj’s picture

duellj CreditAttribution: duellj commented
Status: Active » Needs review
FileSize
entity-disable_pager-1490150-3.patch1.89 KB

Here'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

duellj’s picture

duellj CreditAttribution: duellj commented
FileSize
entity-disable_pager-1490150-4.patch2.25 KB

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -Needs tests
FileSize
entity-disable_pager-1490150-4.patch1.33 KB

Uploading just the test change so we can see it fail.

Status: Needs review » Needs work

The last submitted patch, entity-disable_pager-1490150-4.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/entity/tests/entity_query.testundefined
@@ -1323,6 +1324,27 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+      ->pager(1)

Is there a specific reason why there is two calls to pager()?

duellj’s picture

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

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review

Still needs review for the patch in #4.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

#4 looks RTBC to me, patch is simple, test is good.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Novice

Committed/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 :(

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Patch (to be ported) » Needs review
FileSize
drupal-1490150-12-combined.patch2.01 KB
drupal-1490150-12-tests.patch1.15 KB

The diffstat is off by a line because I didn't include the extra blank line that was in the above patch.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/modules/simpletest/tests/entity_query.testundefined
@@ -1409,6 +1409,27 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+   * Tests the disabling the pager in EntityFieldQuery.

"Tests the disabling the pager"?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 7.x-dev » 8.x-dev
FileSize
d7-entity-pager-1490150-14-do-not-test.patch2.04 KB
d8-entity-pager-1490150-14.patch923 bytes

Something strange going on with the upload. Edit: finally.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Needs work 
+++ b/core/modules/entity/tests/entity_query.testundefined
@@ -1341,7 +1341,7 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+    ), 'All test entities are listed when the pager is disabled and then enabled.', TRUE);

Shouldn't this say "is enabled and then disabled"?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

No?

Edit: Yes.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
FileSize
d8-entity-pager-1490150-20.patch923 bytes
duellj’s picture

duellj CreditAttribution: duellj commented
Status: Needs review » Reviewed & tested by the community

All text changes look good and proper. Thanks xjm!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

Committed and pushed that patch to 8.x. Back to 7.x for the backport, which presumably needs a re-roll to incorporate those fixes.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
FileSize
d7-entity-pager-1490150-23.patch2.04 KB

Didn't I... I guess I didn't.

Here it is.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Looking good!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

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