Notice/warnings on submitting aggregator settings form [#464792]

Navigate to admin/content/aggregator/settings and submit the form. See the errors:

Notice: Undefined index: aggregator_processors in aggregator_admin_form_submit() (line 484 of /Users/akahn/drupal-7/modules/aggregator/aggregator.admin.inc).
Warning: array_filter(): The first argument should be an array in aggregator_admin_form_submit() (line 484 of /Users/akahn/drupal-7/modules/aggregator/aggregator.admin.inc).

Looks like this has to do with aggregator_admin_form_submit():

 function aggregator_admin_form_submit($form, &$form_state) {
  $form_state['values']['aggregator_processors'] = array_filter($form_state['values']['aggregator_processors']);
  system_settings_form_submit($form, $form_state);
}

The issue seems to be that while aggregator_admin_form() checks whether any modules add processors for aggregator, and only displays settings pertaining to these add-ons if they are present, aggregator_admin_form_submit() performs no check and attempts to work with form data that may not be there. (See #303930: Pluggable architecture for aggregator.module for more info.) All that should be needed is adding a similar check to the latter function. Only do the array_filter() if that form element is actually there.

Comment	File	Size	Author
#8	464792-8_aggregator_settings_notices.patch	881 bytes	alex_b
#8
#1	464792-aggregator-settings-errors.patch	909 bytes	akahn
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

akahn CreditAttribution: akahn commented 16 May 2009 at 23:13

File	Size
464792-aggregator-settings-errors.patch	909 bytes

Here's a patch that checks module_implements('aggregator_process'). If the return is an array with a length greater than 1, then the array_filter line gets run. Otherwise, there is only 1 implementation of hook_aggregator_process, which is aggregator_aggregator process, meaning the array_filter line isn't necessary and doesn't get called.

This is the same check that's done for the settings form.

Comment #2

akahn CreditAttribution: akahn commented 16 May 2009 at 23:15

Status:

Active

» Needs review

(anyone know what's up with the comment form eating my comment text when uploading a file?)

Comment #3

dixon_

he/him

English

France

CreditAttribution: dixon_ commented 16 May 2009 at 23:26

Status:

Needs review

» Reviewed & tested by the community

The patch works fine and fixes the problem on a fresh D7 install.

Nothing to comment about code style.

Comment #4

Dries CreditAttribution: Dries commented 17 May 2009 at 05:56

Status:

Reviewed & tested by the community

» Needs work

I think the aggreggator settings should live under 'Site configuration' instead of 'Content management - Aggregator' but that is another issue.

More importantly, can you provide a test with this patch? Either we don't have tests for the settings page yet, or they are not adequate. It would be great if we could look into that.

Comment #5

akahn CreditAttribution: akahn commented 17 May 2009 at 15:12

Dries, would the test just make sure that this form submits without errors? Are there other tests that would be good to follow as a model for this? I haven't written tests before.

And I can make a new issue to move aggregator settings to where they should be: Site configuration.

Comment #6

alex_b CreditAttribution: alex_b commented 18 May 2009 at 14:00

Subscribing

Comment #7

alex_b CreditAttribution: alex_b commented 19 May 2009 at 03:19

Title:

Errors on submitting aggregator settings form

» Notice/warnings on submitting aggregator settings form

#4: #293318: Convert Aggregator feeds into entities adds tests to the settings form (see testFeedContentType()) - could we fix the notice/warnings with a patch as suggested in #1 and add the tests with #293318?

If we really do want to have the tests with this patch, we should take them from #293318 - starting with testFeedContentType().

Comment #8

alex_b CreditAttribution: alex_b commented 28 March 2010 at 23:55

Status:	Needs work	» Needs review
Issue tags:		+Quick fix

File	Size
464792-8_aggregator_settings_notices.patch	881 bytes

I've simplified the fix as it's really just a check for the presence of a value (that *is* an array if it is present) before running an array_filter() on it (see patch).