admin/content filters are broken

Something is funny with the inheritance of the #submit property in the form, I haven't yet been able to figure out what has changed to break this in the core FormAPI.

But, adding #submit properties to each of the buttons appears to work and I've also written a test case for the node administration page and filter functionality.

- Is adding #submit to each button the way to go?
- Should node_filter_form_submit() be split up into smaller _submit functions to remove the 'op' switch?

I'm reviewing this patch...

I applied the test, and it worked, as well as the SimpleTests. I tested the filter on admin/content and am getting the correct results back. This forcibly adds the submit function to the buttons, so that seems fine to me. I don't see a reason to split up the node_filter_form_submit() function, so this patch looks good to go.

Great! Those tests successfully fail (heh ;)) when the first hunk is not applied.

Some minor nitpicks, and then this is good to go.

+++ modules/node/node.admin.inc	15 Aug 2009 09:38:55 -0000
@@ -145,7 +145,6 @@ function node_filter_form() {
-  $form['#submit'][] = 'node_filter_form_submit';

I don't understand why this wasn't sufficient. Could you place a one-liner above the button array definitions that explains why we're doing #submit at the button level rather than the form level? I could see someone trying to "optimize" this later.

+++ modules/node/node.test	15 Aug 2009 11:47:02 -0000
@@ -840,3 +840,54 @@ class NodeAccessRebuildTestCase extends 
+  protected $web_user;

Elsewhere we use $web_user to refer to an un-privileged user, and $admin_user to refer to an administrator. "administer nodes" is a pretty big permission, so I'd rename to $admin_user.

+++ modules/node/node.test	15 Aug 2009 11:47:02 -0000
@@ -840,3 +840,54 @@ class NodeAccessRebuildTestCase extends 
+  function testNodeAdmin() {

Please add a line of PHPDoc here.

+++ modules/node/node.test	15 Aug 2009 11:47:02 -0000
@@ -840,3 +840,54 @@ class NodeAccessRebuildTestCase extends 
+    $this->assertRaw(t('<strong>%a</strong> is <strong>%b</strong>', array('%a' => t('status'), '%b' => t('published'))), t('The node administration listing is filtered by status.'));
...
+    $this->assertRaw(t('<strong>%a</strong> is <strong>%b</strong>', array('%a' => t('status'), '%b' => t('published'))), t('The node administration listing is filtered by status.'));
+    $this->assertRaw(t('<strong>%a</strong> is <strong>%b</strong>', array('%a' => t('type'), '%b' => 'Article')), t('The node administration listing is filtered by content type.'));

Could we use sensible placeholders here rather than %a and %b?

I'm on crack. Are you, too?

I've figured out the actual cause of this bug. As a result of #394702: Admin/content/node should have a create content link, the filter form was moved down one level in the page form array hierarchy. ie $form = node_filter_form(); to $form[] = node_filter_form();.
This moved the #submit property for the filter form from $form['#submit'] to $form[0]['#submit'].

It appears that a form (ie non-button) #submit property can only be located at the root level of the form array.

Could we use sensible placeholders here rather than %a and %b?

Well, that was copy/pasted directly from node_filter_form():

$form['filters']['current'][] = array('#markup' => t('<strong>%a</strong> is <strong>%b</strong>', array('%a' => $filters[$type]['title'], '%b' => $value)));

Should this be rectified as well?

Instead of inserting button-level submit handlers, just moving the #submit from node_filter_form() to node_admin_content() works as well.

Could we use sensible placeholders here rather than %a and %b?

How about changing %a to %type and %b to %value.

Patch has these changes for the test and the original code in node_filter_form()

Great, thanks a lot! Committed to HEAD. :)