Attempt to loop through undefined result set in aggregator [#1058754]

Hi everyone,
My first time logging a bug in Drupal core, so aplogies if I get anything wrong here. I've looked through the 7.x issue queue and can't see this listed yet.

Basically, theres a PHP error which results in an error message after a certain sequence of using aggregator blocks.
Steps to reproduce this:
1) Add several feeds to the aggregator with settings to allow say 5 items in their blocks
2) Assign one or more of those feed blocks to a region in your theme
3) Then go back to the aggregator and set the 'News items in block' setting for every feed to zero (to disable their blocks)
4) You see an error message on every page that the blocks previously appeared on. Something like:

Notice: Undefined variable: result in aggregator_block_view() (line 418 of /httpd.www/drupal/modules/aggregator/aggregator.module).
# Warning: Invalid argument supplied for foreach() in aggregator_block_view() (line 418 of /httpd.www/drupal/modules/aggregator/aggregator.module).

The relevant bit of code is the function aggregator_block_view in lines 394-426 in aggregator.module:

function aggregator_block_view($delta = '') {
  if (user_access('access news feeds')) {
    $block = array();
    list($type, $id) = explode('-', $delta);
    switch ($type) {
      case 'feed':
        if ($feed = db_query('SELECT fid, title, block FROM {aggregator_feed} WHERE block <> 0 AND fid = :fid', array(':fid' => $id))->fetchObject()) {
          $block['subject'] = check_plain($feed->title);
          $result = db_query_range("SELECT * FROM {aggregator_item} WHERE fid = :fid ORDER BY timestamp DESC, iid DESC", 0, $feed->block, array(':fid' => $id));
          $read_more = theme('more_link', array('url' => 'aggregator/sources/' . $feed->fid, 'title' => t("View this feed's recent news.")));
        }
        break;

      case 'category':
        if ($category = db_query('SELECT cid, title, block FROM {aggregator_category} WHERE cid = :cid', array(':cid' => $id))->fetchObject()) {
          $block['subject'] = check_plain($category->title);
          $result = db_query_range('SELECT i.* FROM {aggregator_category_item} ci LEFT JOIN {aggregator_item} i ON ci.iid = i.iid WHERE ci.cid = :cid ORDER BY i.timestamp DESC, i.iid DESC', 0, $category->block, array(':cid' => $category->cid));
          $read_more = theme('more_link', array('url' => 'aggregator/categories/' . $category->cid, 'title' => t("View this category's recent news.")));
        }
        break;
    }
    $items = array();
    foreach ($result as $item) {
      $items[] = theme('aggregator_block_item', array('item' => $item));
    }

    // Only display the block if there are items to show.
    if (count($items) > 0) {
      $block['content'] = theme('item_list', array('items' => $items)) . $read_more;
    }
    return $block;
  }
}

I think what is happening here, is that the aggregator_block_view function is still running on those pages, and the SQL is returning no items, since there are no feeds with block items>0. The variable $result is then not set, and on line 416, attempting to loop through the $result array generates an error.

This could be solved with an if(isset($result)) {} wrapper around the for loop?

Comment	File	Size	Author
#31	drupal-1058754-31-test.patch	1.64 KB	blackra
#27	drupal-1058754-27-test-only.patch	1 KB	tim.plunkett
#27	drupal-1058754-27-combined.patch	2.03 KB	tim.plunkett
#23	aggregator-1058754-23-test.patch	1.02 KB	xjm
#23	aggregator-1058754-23-combined.patch	2.07 KB	xjm
#21	aggregator-test-1058754-21-d8.patch	1.02 KB	naxoc
#21	aggregator-module-1058754-21-d8.patch	1.05 KB	naxoc
#21	aggregator-test-1058754-21-d7.patch	1 KB	naxoc
#21	aggregator-module-1058754-21-d7.patch	1.01 KB	naxoc
#15	aggregator_isset-1058754-15.patch	658 bytes	kathyh
#10	aggregator_isset-1058754-8.patch	634 bytes	Yaron Tal
#10	aggregator_isset-1058754-8-D7.patch	634 bytes	Yaron Tal
#4	aggregator_1058754.patch	636 bytes	Yaron Tal

Comments

Comment #1

grahamgilchrist commented 14 February 2011 at 11:39

Version:

7.x-dev

» 7.0

Comment #2

Yaron Tal commented 14 February 2011 at 13:19

looks like the value of $type isn't in the switch, because $result should contain "DatabaseStatementInterface A prepared statement object, already executed.", and not like in 6.x a FALSE. A default in the switch could solve this I guess.

What about adding this:

switch ($type) {
  default:
    return array('content' => '', 'subject' => '');
    break;
}

edit: or $id isn't a valid id. Anyhow, the $delta seems corrupt.

Comment #3

grahamgilchrist commented 14 February 2011 at 13:48

Debugging on my site seems to indicate that the $type switch does get set properly.
It switches into $case 'feed' without any problems, but the issue seems to be if $feed returns nothing, then $result is not set, and then the foreach loop throws an error as $result is not an array.

This only happens if you assign a feed block to a region, and then set the 'News items in block' setting (at admin/config/services/aggregator/edit/feed/#id) to zero for all feed blocks.

Comment #4

Yaron Tal commented 14 February 2011 at 14:28

Status	File	Size
new	aggregator_1058754.patch	636 bytes

oops sorry yeah you are right. I missed the <> 0 in the first sql. I agree that adding if (isset($result)) should be enough to fix the issue. Also this would fix the hypothetical issue of a corrupt $delta.

Comment #5

Yaron Tal commented 14 February 2011 at 14:28

Status:

Active

» Needs review

Comment #6

libre fan commented 3 October 2011 at 16:37

Hello,

I'm getting

Notice: Undefined variable: result in aggregator_block_view() (line 415 of /modules/aggregator/aggregator.module).
    Warning: Invalid argument supplied for foreach() in aggregator_block_view() (line 415 of /modules/aggregator/aggregator.module).

even if I don't assign zero for all feed blocks.

It's a recent error though. It happened after I added a new news feed and a new feed block.

Comment #7

Yaron Tal commented 5 October 2011 at 06:31

@#6 did you try using the above patch? It looks like the exact same error. Giving only 1 block a 0 should be enough to trigger it.

Comment #8

twistor commented 6 October 2011 at 08:25

Version:	7.0	» 8.x-dev
Status:	Needs review	» Needs work

The patch seems fine, but there's a whitespace problem. Moving to 8.x as it exists there.

Comment #9

libre fan commented 7 October 2011 at 08:03

@#6 Many thanks, Yaron Tal, for your patch. I wasn't sure if it was safe to use for anybody. But appparently it works on my site.

Comment #10

Yaron Tal commented 7 October 2011 at 10:49

Status:

Needs work

» Needs review

Status	File	Size
new	aggregator_isset-1058754-8-D7.patch	634 bytes
new	aggregator_isset-1058754-8.patch	634 bytes

I attached a D8 version of the patch and a D7 version with the whitespace problem fixed.

Comment #11

7 October 2011 at 10:50

Status:

Needs review

» Needs work

The last submitted patch, aggregator_isset-1058754-8.patch, failed testing.

Comment #12

Yaron Tal commented 7 October 2011 at 11:32

Status:

Needs work

» Needs review

#10: aggregator_isset-1058754-8.patch queued for re-testing.

Comment #13

7 October 2011 at 11:33

Status:

Needs review

» Needs work

The last submitted patch, aggregator_isset-1058754-8.patch, failed testing.

Comment #14

libre fan commented 18 November 2011 at 14:47

Hello everybody,

Apparently the patch didn't make its way into Drupal 7.9, so I reapplied patch aggregator_1058754.patch

Comment #15

kathyh commented 27 November 2011 at 14:36

Status:

Needs work

» Needs review

Status	File	Size
new	aggregator_isset-1058754-15.patch	658 bytes

Updated patch for D8 /core changes.

Comment #16

tamsoftware commented 23 January 2012 at 13:23

D7 patch worked beautifully here! THANKS!

Details:
Bug surfaced in Drupal 7.8 when setting unused blocs to zero
Upgrading to 7.10 did not clear the issue
Applying the #10 patch like this fixed it:

#cd drupal
#patch -p1 modules/aggregator/aggregator.module  aggregator_isset-1058754-8-D7.patch

Franck Horlaville
TAM Software

Comment #17

libre fan commented 19 February 2012 at 18:36

hello everybody,

Just wondering why this useful patch hasn't find its way into Drupal 7.12

Comment #18

jody lynn

she/her

English

commented 20 February 2012 at 00:15

Status:

Needs review

» Reviewed & tested by the community

Looks fine.

Comment #19

catch

he/him

English

commented 20 February 2012 at 07:11

Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs tests

This could use a test.

Comment #20

naxoc commented 29 February 2012 at 18:03

Assigned:

Unassigned

» naxoc

I'll look at a test for this.

Comment #21

naxoc commented 29 February 2012 at 22:12

Status:

Needs work

» Needs review

Status	File	Size
new	aggregator-module-1058754-21-d7.patch	1.01 KB
new	aggregator-test-1058754-21-d7.patch	1 KB
new	aggregator-module-1058754-21-d8.patch	1.05 KB
new	aggregator-test-1058754-21-d8.patch	1.02 KB

I made the tests separate patches so that the tests can run without the fix and fail on purpose. That way we can "test the test".

I changed the fix a little so we avoid the use of an uninitialized variable.

There are patches for both D7 and D8

Comment #22

29 February 2012 at 22:42

Status:

Needs review

» Needs work

The last submitted patch, aggregator-module-1058754-21-d7.patch, failed testing.

Comment #23

xjm

she/her

English

commented 9 April 2012 at 13:19

Issue tags:

-Needs tests

Status	File	Size
new	aggregator-1058754-23-combined.patch	2.07 KB
new	aggregator-1058754-23-test.patch	1.02 KB

That test looks great. Thanks @naxoc!

Here I've made one small change (removed t() from the assertion message; see http://drupal.org/simpletest-tutorial-drupal7#t). @naxoc's post above demonstrates that the test fails correctly without the fix. I combined the fix with the test in the second patch so we can confirm the fix does fix it.

Comment #24

xjm

she/her

English

commented 9 April 2012 at 13:19

Status:

Needs work

» Needs review

Comment #25

xjm

she/her

English

commented 9 April 2012 at 13:59

Status:

Needs review

» Reviewed & tested by the community

And, I think that's RTBC. :)

Comment #26

catch

he/him

English

commented 13 April 2012 at 07:16

Title:	Attempt to loop through empty array in Aggregator block view	» Attempt to loop through undefined result set in aggregator
Version:	8.x-dev	» 7.x-dev
Status:	Reviewed & tested by the community	» Patch (to be ported)
Issue tags:		+Novice, +Needs backport to D7

Committed/pushed to 8.x, thanks!

Moving to D7 for backport. Should be a straightforward backport so tagging novice for that.

Comment #27

tim.plunkett

he/him

English

Philadelphia

commented 17 April 2012 at 03:34

Status:

Patch (to be ported)

» Needs review

Status	File	Size
new	drupal-1058754-27-combined.patch	2.03 KB
new	drupal-1058754-27-test-only.patch	1 KB

Straight backport with patch -p2, didn't test it myself.

Comment #28

xjm

she/her

English

commented 17 April 2012 at 10:03

Assigned:	naxoc	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

Correct diffstat; correct code; nothing D8 specific. I swear! :)

Comment #29

webchick

she/they

English

Vancouver 🇨🇦

commented 29 April 2012 at 06:09

Status:

Reviewed & tested by the community

» Fixed

Committed and pushed to 7.x for parity with 8.x. Thanks!

However, question. Is t($block['title']) kosher? I've always been slapped by Gábor for using t() with dynamic values.

Comment #30

xjm

she/her

English

commented 30 April 2012 at 17:56

Version:	7.x-dev	» 8.x-dev
Status:	Fixed	» Needs work

You're right; that's totally wrong. It should probably be an assertRaw(), I think? It's in any case not matching the correct thing, because the original string is definitely not output as just that value inside its own t(). Reference:
http://drupal.org/simpletest-tutorial-drupal7#t

Novice task: Followup patch change the assertion to something more correct. :)

Comment #31

blackra commented 27 August 2012 at 11:47

Status	File	Size
new	drupal-1058754-31-test.patch	1.64 KB

Here is a patch that includes the following:

Changed assertNoText() to assertNoRaw() in the check for the empty feed case
Removed the t() from the assertNoRaw() test and the corresponding assertText() test (for where there is content)
Added comments to help keep these tests in sync

Things I did not do:

Change assertRaw() to assertText() for the content present case. I was not sure whether this was appropriate and leaving it seemed safe since assertText() should be the tougher test.
Find a feed name that would differentiate between t() and no t()

In the process of looking for a feed name that would differentiate between t() and no t(), I found that changing $feed_name = $this->randomName(10); to $feed_name = 'hello%20&raw'; in AggregatorTestBase.php produced lots of test failures. I haven't worked out why yet. However, it appears to be possible to set up a feed with this name through the user interface.

Comment #32

blackra commented 27 August 2012 at 11:49

Status:

Needs work

» Needs review

Comment #33

cameron tod commented 9 September 2012 at 15:30

rob.black, thank you very much for your help! Could you roll the patch #27 & #31 together into a combined patch, one with the tests only, and one with the fixes + tests? That way the whole patch can be reviewed, tested by testbot, and if all goes well, committed.

Comment #34

cameron tod commented 9 September 2012 at 15:30

Status:

Needs review

» Needs work

Comment #35

blackra commented 9 September 2012 at 21:13

cam8001, I am confused.

I was attempting to produce a patch to address xjm and webchick's comments in 8.x based on the understanding that the original patch #23 had already been committed and, after it was applied the tests were restructured. Since #23 has already been committed, there is no point rolling my patch together with it; my patch is based on a version of 8.x that already includes that fix.

#27 is a 7.x backport patch (and again, I think it has already been committed). Whilst I could produce a 7.x backport of my patch, shouldn't this wait until the 8.x version has been reviewed?