node_admin_nodes_validate is never called when clicking update in admin/content

subscribe

This patch should do it ... please to review

Good work at fixing the problem, but I think it would be good if we gave the user some feedback rather than just dumping them back where they started.

I have, therefore, added a couple of lines to the patch which display a warning message that "No nodes were selected" and then returns - if the return line is removed then node_mass_update() is called, which itself displays a message saying that nodes have been updated - thus contradicting themessage I have added.

Sorry, the tabs weren't right in that code. Fixed here.

I confirm that jim0203's patch (a) works and (b) is more user friendly than the one I posted above...

The white space was a bit weird (this patch fixes it) ... but other than that, its good to go ...

forgot to change "status".

@redndahead ... an excellent suggestion. see patch

New patch, incorporating suggestions in #10

**IGNORE THIS** ... I hadn't refreshed the page and seen that someone else already submitted a patch that passed

I've tested "empty_update-609152-6.patch" and it works fine ... I think we are ready to go.

Minor nit-pick. We don't say "node" anywhere in user-facing text. It should be 'content' instead. Feel free to mark back to RTBC when this is fixed.

+++ modules/node/node.admin.inc	24 Oct 2009 05:58:22 -0000
@@ -489,6 +489,13 @@ function node_admin_nodes_validate($form
   $operations = module_invoke_all('node_operations');
   $operation = $operations[$form_state['values']['operation']];
+  
+  // If no content is selected tell the user and return.
+  if (!is_array($form_state['values']['nodes'])) {
+    drupal_set_message('No content was selected.', 'warning');
+    return;
+  }
+

Hmmm. And now that I look at this again, let's actually move this new logic up to the very top of the function. There's no point in invoking node_operations hooks if they didn't select any content.

This review is powered by Dreditor.

Hm. And in looking at this /again/ there's something else funny going on here.

a) We should not be doing checks like this in a submit handler. Submit handlers should be able to assume that everything's hunky-dory.

b) Where this kind of logic belongs is in the validate handler for a form.

c) However, there already /is/ a validate handler in the form, which is basically doing exactly this:

function node_admin_nodes_validate($form, &$form_state) {
  $nodes = array_filter($form_state['values']['nodes']);
  if (count($nodes) == 0) {
    form_set_error('', t('No items selected.'));
  }
}

So the real problem is figuring out why that code isn't firing.

Update from IRC:

+++ modules/node/node.admin.inc	24 Oct 2009 17:42:00 -0000
@@ -474,10 +474,19 @@ function node_admin_nodes() {
+  if ($form_state['clicked_button']['#value'] == 'Update') {

The problem with code like this is that it doesn't work when my site is in French, and 'Update' becomes 'Réviser' or whatever.

While wrapping 'Update' in t() can mitigate that, this also breaks if a front-end developer decides to go in and form_alter() it to a different label altogether, not going through the translation system.

A better way to go would be to attach the validate handler to the button itself. I looked around in core briefly for an example and didn't find one, but basically it would entail:

[13:56]  <webchick> 1. Rename the function to something that won't auto-trigger when the form is submitted (like node_admin_nodes_update_validate)
[13:56]  <webchick> 2. In the definition of the submit button, add:
[13:57]  <webchick> '#validate' => 'node_admin_nodes_update_validate'
(or something like that)

Also, since this has to be the biggest disparity severity of problem : back-and-forth to fix it ratio I've seen on an issue in a long time, I think we officially need some tests for this so we don't ever break it again. :)

This review is powered by Dreditor.

Ok! Last (hopefully) things to change. :) Great job on the perseverance here!

+++ modules/node/node.admin.inc	24 Oct 2009 18:38:44 -0000
@@ -475,6 +476,13 @@ function node_admin_nodes() {
+  // Error if there are no items to select.
+  if (!is_array($form_state['values']['nodes'])) {
+    form_set_error('', t('No items selected.'));
+    return;
+  }
+
+  // Error if there are no items selected.
   $nodes = array_filter($form_state['values']['nodes']);
   if (count($nodes) == 0) {
     form_set_error('', t('No items selected.'));

1. Let's condense these two things down to one check. In other words:

if (!is_array(..) || count(...) == 0) {
...
}

2. Don't return; here. We want all of the validation routines to fire. Errors are accumulated in form_set_error().

+++ modules/node/node.test	24 Oct 2009 18:38:45 -0000
@@ -967,6 +967,9 @@ class NodeAdminTestCase extends DrupalWe
+    $this->drupalPost('admin/content', array(), t('Update'));
+    $this->assertText(t('No items selected.'), t('Clicking update with no nodes displays error message on the node administration listing.'));

This is good, and checks for the situation where there are no nodes to select. We should also check for the condition where there ARE nodes but none are selected.

I'm on crack. Are you, too?

By jove, I think you've got it! :) I can officially find nothing else to complain about now, so committed to HEAD!

Great work! :D

#35 works in Drupal 6.15

Has this been fixed in the latest D6?

nope

If #35 works with D6, can you please post a copy of the patch under 6.x-dev?

rerolled #35 using git

Sorry bleen18, your patch was ignored because of the filename I think. Try this format:

[description]-[issue-number]-[comment-number].patch

from http://drupal.org/node/1054616

Try resubmitting the patch and changing the status back to "needs review".