Multiple pager support is partially broken. [#903110]

The default value of $element in pager.inc is NULL.
And, if it stays null (nobody calls ->element($element)), we just generate the element number ourselves.
However, the following empty check is wrong:

  /**
   * Ensure that there is an element associated with this query.
   *
   * After running this query, access $this->element to get the element for this
   * query.
   */
  protected function ensureElement() {
    if (!empty($this->element)) {
      return;
    }

    $this->element = self::$maxElement++;
  }

The problem is that empty() is true if $this->element == 0, and 0 is the first pager element, which is perfectly valid. This means that even though we specified a valid element (0), the code generates it's own instead of returning at the beginning of the function.

This becomes a problem when writing tests, I'm creating multiple EntityFieldQuery objects (now with pager support), and EntityFieldQuery calls the pager, which instead of using element 0 for each test increments it, and tries using element 1, element 2, element 3, which fails...

Comment	File	Size	Author
#19	903110.patch	3.57 KB	bojanz
#19
#18	903110.patch	3.61 KB	bojanz
#18
#16	903110.patch	3.52 KB	bojanz
#16
#10	903110.patch	3.42 KB	bojanz
#10
#9	903110.patch	2.96 KB	bojanz
#9
#5	903110.patch	1.54 KB	bojanz
#5
#4	903110.patch	527 bytes	bojanz
#4
#1	903110.patch	530 bytes	bojanz
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

bojanz CreditAttribution: bojanz commented 5 September 2010 at 23:29

Status:

Active

» Needs review

File	Size
903110.patch	530 bytes

Here's a patch. empty() is replaced with is_null().

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 5 September 2010 at 23:32

Status:

Needs review

» Needs work

is_null() is always isset()

Comment #3

bojanz CreditAttribution: bojanz commented 5 September 2010 at 23:49

Status:

Needs work

» Needs review

Edit: Okay, did a search, isset is faster and thus recommended. The fact that core uses is_null in some places does not make it cool.

Comment #4

bojanz CreditAttribution: bojanz commented 5 September 2010 at 23:56

File	Size
903110.patch	527 bytes

New patch.

Comment #5

bojanz CreditAttribution: bojanz commented 6 September 2010 at 18:58

File	Size
903110.patch	1.54 KB

Talked with chx on IRC, and he found another problem:
self::$maxElement is not incremented when $element is set via element().

The problem illustrated:
1) Add three pagers, specify elements (0, 1, 2)
2) Now add a pager, don't specify the element. Since $maxElement was not updated, it tries to use 0, which is wrong.
This works for any multiple number of pagers.

I'm including a better patch. Ideally, this should have tests, but I'm not sure how, since $element is a protected property (the pager tests could use some loving in any case...)

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 6 September 2010 at 19:12

+++ includes/pager.inc	6 Sep 2010 18:28:10 -0000
@@ -149,10 +149,16 @@ class PagerDefault extends SelectQueryEx
+    if ($element >= self::$maxElement) {
+      self::$maxElement = ++$element;

I think this should be just >, and we want to set $maxElement = $element

Or something along those lines.

I wonder whether one of those functions shouldn't call the other, so there's just one place where the value of $maxElement is managed?

Powered by Dreditor.

Comment #7

bojanz CreditAttribution: bojanz commented 6 September 2010 at 19:17

Well, ensureElement() uses the current value of $maxElement, and THEN increments it.
That way the $element can start from 0 and not 1.

So if I set ->element(5) with your suggested changes in place, ensureElement would try to use 5 as the next element, which is wrong.

Btw, the patch is still not ready, talked with chx about how we could test this, and the tests are totally doable.

Comment #8

chx CreditAttribution: chx commented 8 September 2010 at 05:31

    if ($element >= self::$maxElement) {
      self::$maxElement = ++$element;
    }

can just be self::$maxElement = $element + 1; you do not use $element later why increment it.

Comment #9

bojanz CreditAttribution: bojanz commented 8 September 2010 at 23:46

File	Size
903110.patch	2.96 KB

True.
Changed that, and added a test. Confirmed that test fails with old pager.inc, succeeds with the new one.

Comment #10

bojanz CreditAttribution: bojanz commented 9 September 2010 at 00:19

File	Size
903110.patch	3.42 KB

Made the tests more complete, this should be it.

Comment #11

bojanz CreditAttribution: bojanz commented 9 September 2010 at 10:26

Title:

Empty check in pager.inc is wrong

» Multiple pager support is partially broken.

Changing the title to better reflect our problem.

Comment #12

chx CreditAttribution: chx commented 9 September 2010 at 13:37

Component:

base system

» database system

Moving to database hoping it will invoke Crell, it's his tests anyways. I can review a bit later too.

Comment #13

Crell CreditAttribution: Crell commented 9 September 2010 at 15:34

module_invoke_all('Crell', $patch);

#10 makes sense to me. The only place I can think of where it would still break is if you explicitly set an element value that has already been used. What do we want to happen there? Blindly use it and hope the developer knows what he's using? (That's what it's doing now, and arguably what we've always done there.) Return a different element instead? (I'm sure chx will balk at that since that's the reason for the non-chainability of some SelectQuery methods.)

Comment #14

bojanz CreditAttribution: bojanz commented 9 September 2010 at 15:42

Thanks for the quick review.

The only place I can think of where it would still break is if you explicitly set an element value that has already been used. What do we want to happen there? Blindly use it and hope the developer knows what he's using?

Yes. If you specify a used element, the $maxElement counter doesn't get incremented, and it's used blindly.
It does exactly what is expected, no need to work around incorrect usage.
I actually use it that way in the EFQ (pager/tablesort) tests, since there's no need to have 10 elements for 10 tests.

That's what it's doing now, and arguably what we've always done there.

Then we agree that it's an okay approach to take here?

Comment #15

Crell CreditAttribution: Crell commented 9 September 2010 at 15:48

Let's document that fact in the docblock, just to be explicit about it. Then we should be good to go.

Comment #16

bojanz CreditAttribution: bojanz commented 9 September 2010 at 16:01

File	Size
903110.patch	3.52 KB

Here you go.

Comment #17

Crell CreditAttribution: Crell commented 9 September 2010 at 19:17

"Note that no auto-correction is done when setting an existing element ID, it is used as is." - That's a comma splice. :-) I'd suggest something like:

"Note that no collision detection is done when setting an element ID explicitly so it is possible for two pagers to end up using the same ID if both are set explicitly."

That warns people of exactly what the net effect is.

Comment #18

bojanz CreditAttribution: bojanz commented 9 September 2010 at 21:13

File	Size
903110.patch	3.61 KB

I was wandering how to make that clearer, but couldn't find you on IRC.
Updated the patch.

An alternative could be "Note that no collision detection is done when setting an element ID explicitly, so it is possible for two pagers to end up using the same ID." which sounds less redundant, but it doesn't matter much.