aggregator categorize feature appears to be broken in drupal HEAD (just downloaded and installed). I have this errors when tryng to access
drupal/aggregator/sources/1/categorize

Notice: Trying to get property of non-object in aggregator_page_source() (line 39 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_aggregator_feed_source() (line 491 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_aggregator_feed_source() (line 491 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_aggregator_feed_source() (line 492 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_aggregator_feed_source() (line 493 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_aggregator_feed_source() (line 494 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_aggregator_feed_source() (line 496 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Notice: Trying to get property of non-object in aggregator_feed_items_load() (line 93 of /Applications/MAMP/htdocs/drupal/modules/aggregator/aggregator.pages.inc).
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mr.baileys’s picture

mr.baileys’s picture

Status: Active » Needs review
FileSize
4.12 KB

The aggregator module uses aggregegator_page_category() and aggregator_page_source() both as normal callbacks as well as through a drupal_get_form callback. This leads to a strange function signature:

function aggregator_page_category($arg1, $arg2 = NULL) {
  // If there are two arguments then we are called as a form, $arg1 is
  // $form_state and $arg2 is $category. Otherwise, $arg1 is $category.
  $category = is_array($arg2) ? $arg2 : $arg1;

The fact that $form was added to the drupal_get_form callback signature causes this to fail.

I'd opt to split the function into a regular callback and a callback for drupal_get_form. This fixes the issue, and is more developer friendly.

mr.baileys’s picture

FileSize
4.45 KB

Marked #725640: Module fails to load RSS Feed with errors as a duplicate. Improved the doxygen comments in the previous patch some more.

Status: Needs review » Needs work

The last submitted patch, aggregator.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review

#3: aggregator.patch queued for re-testing.

catch’s picture

Patch looks sane enough. Could we add a quick test - just visiting the path ought to be enough to trigger it no?

mr.baileys’s picture

We could add tests (you are right, a simple page visit will trigger the notices), but I'd prefer to have this handled by #276486: Tests needed: aggregator.admin.inc and #276499: Tests needed: aggregator.pages.inc to prevent scattered bits and pieces of tests. On the other hand, those two issues haven't seen much movement lately.

dodorama’s picture

I applied the patch everything works now.

catch’s picture

I've actually been closing out those "tests needed" issues because they're usually so stale that there's no useful summary of what's actually missing or not - since we moved to adding tests when and if rather than just trying to get coverage up overall, but this could probably go in as a quick fix as well.

catch’s picture

FileSize
55.13 KB

Here's the test.

edit: this was supposed to be just the test by itself, but other patches got in with it, however the 8 exceptions are the ones we wanted to see. Following post is the patch + the test together.

catch’s picture

FileSize
5.42 KB

And the patch + test combined.

mr.baileys’s picture

Thanks catch! Test looks good to me, and confirmed that the test correctly catches the bug without the rest of the patch applied. Now we just need someone else to review & RTBC...

catch’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed #3, mr.baileys reviewed #16. Test bot reviewed both, I think that's a quorum.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Good fix. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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