pager_load_array() tries to make sure the array is properly initialized. Except its test for initialization is NOT notice safe. Attach patch replaces a ! with an empty() check which fixes the notice.

This is easily tested with any method you have to set a pager element != 0; the easiest way is to do something silly like hack node_page_default but you can also use Views and change the pager element in a view, then view the page.

CommentFileSizeAuthor
#22 drupal-pager_notice-266434-22.patch448 bytespdrake
#18 drupal.pager-notice.patch599 bytessun
#7 pager.notice.patch1.15 KBcatch
#6 pager.notice.patch1.15 KBcatch
#3 pager.notice.fix_.patch1.11 KBmerlinofchaos
pager.notice.fix_.patch598 bytesmerlinofchaos

Comments

dropcube’s picture

dropcube commented
Version: 6.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

The patch fixes this, so RTBC.

dropcube’s picture

dropcube commented
Status: Reviewed & tested by the community » Needs work

Oh, didn't change the element in both pager_query and theme('pager', ...). Now I get notice: Undefined offset: 10 in \wwwroot\drupal7\includes\pager.inc

merlinofchaos’s picture

merlinofchaos commented
Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review
StatusFileSize
new pager.notice.fix_.patch1.11 KB

Try this version.

dropcube’s picture

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

Ok, It now works with $element > 0 .

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Needs work

Why use !isset in one place and empty in another? Can we be consistent and use the same test?

catch’s picture

catch
he/him
Primary language English
commented
Version: 6.x-dev » 7.x-dev
Status: Needs work » Needs review
StatusFileSize
new pager.notice.patch1.15 KB

Stilll applies against HEAD, rerolled with isset().

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new pager.notice.patch1.15 KB

Code style error.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Even the first patch from 2008 passed. ;)

webchick’s picture

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

Yes. Now let's get some tests so this doesn't ever break again.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +Needs tests

tagging.

merlinofchaos’s picture

merlinofchaos commented

I can't believe this issue, a 4 line patch, has been sitting for 15 months, because it was sent into the 'needs work' wasteland over a philosophy issue. earnie, WTF. Seriously. Thank you for standing in the way of a perfectly useful fix.

And tests? Why the hell is this notice fix being held up on tests?

merlinofchaos’s picture

merlinofchaos commented
Status: Needs work » Needs review

Putting back to needs review so that somebody might actually see this before 2011.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Anonymous (not verified) commented

earnie, WTF. Seriously. Thank you for standing in the way of a perfectly useful fix.

@merlinofchaos: Really, it isn't me if you read the comments after mine. And it appears you need to fix the issues for the testbot. Result Failed: Failed to apply patch.

merlinofchaos’s picture

merlinofchaos commented

earnie, you set it needs work in comment #5 June 2008 and no one even saw this again until May of 2009, so don't tell me it wasn't you. You took a perfectly valid patch and set it 'needs work' because you wanted to see isset() instead of !empty(). 'needs work' means nobody else will ever look at it.

in 2008 there was no testbot. Frankly, I think you should go fix the tests. It's your fault that a useful fix has sat for 15 months. The only reason I went to look for the issue is that I noticed that I still have this patch against one of my dev sites.

damien tournoud’s picture

damien tournoud commented

Ranting surely helps getting things done.

merlinofchaos’s picture

merlinofchaos commented

Damien: Well, it got someone else to actually see it, which is something. Of course, the stupid patch doesn't apply now, it appears. Sigh.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.pager-notice.patch599 bytes

Seriously, I have no idea how to test this.

pager_load_array() is only used internally by pager.inc itself, as far as Drupal core is concerned. The second fix from the original patch for pager_query() already went in as part of another patch, so the PagerDefault query extender already sets an unset $element now.

Since the previous condition equaled empty() and not !isset(), I'm using empty() in this patch. !isset() would only work for NULL, not for empty strings or the like.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Seems like no one else has an idea how to test this. Can we get it in?

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed! Finally. ;-)

Status: Fixed » Closed (fixed)

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

pdrake’s picture

pdrake commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review
StatusFileSize
new drupal-pager_notice-266434-22.patch448 bytes

This notice also occurs in D6. Attached is a patch for D6 containing the fix committed in D7.

Peter Bowey’s picture

Peter Bowey commented

Refer #22

This 'needed' fix is still not present in the latest D6 Pressflow 6.25!
I have not verified if it exists in Drupal D6 6.25 [non-Pressflow]

It is now the 25th April 2012 ;-)

Hi @DavidStrauss, @FourKitchens

Rhino-new’s picture

Rhino-new commented

Still not available in Pressflow, thank goodness I found it here.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.