Scheduler shouldn't require a date when clicking the "Delete" button

I have this problem as well.

To elaborate a little: If you have a node which requires a scheduled publish date, but you have not filled in a date, you cannot delete the node without first putting in a publish date some time in the future.

This is a problem when you edit a node type and add the scheduler requirement, then all previously published nodes of that type cannot be deleted without adding a scheduled date first. Annoying and doesn't make sense.

Yes that is a problem. I think it is similar to #1198788: Editing a published node with "Publishing date/time is required" forces users to set a new "Publish on" date/time about not validating the date/time when you just want to edit the node after it is already published. There is a patch in that issue - maybe it can be worked to cover both scenarios?

Jonathan

I did not know this before, but after a bit of searching I found the excellent feature of the form API in D7 #limit_validation_errors which limits validation to specific sections depending on the button clicked. So in scheduler_form_alter we only need to add:

    // Do not perform any validation when the Delete button is clicked.
    $form['actions']['delete']['#limit_validation_errors'] = array();

Here is a patch, against 7.x-1.1+10

Jonathan

Be careful because this patch disables ALL validation. There are use cases where validation is absolutely necessary on the delete action. Example:

Are you really sure you want to be removed from the list for a donor heart? Type the word "DELETE" below to proceed.

:-)

Yes all field validation is bypassed, but the user still gets the required "Are you sure you want to delete ... this action cannot be undone" message, and they need to click 'Delete' again.

I cannot think of any validation which should be done on data fields if the node is about to be deleted.

Jonathan

Well, the use case I described above is a pattern that I have seen before. I can come up with more :) What about a site that requires a captcha to be filled in to prevent mass deleting by a bot? Or a site that requires editors to provide a log message when they are deleting content? A frontend for an asynchronous backup tool which is only allowed to delete a backup job when it has been flagged with "backup successfully completed"?

It's impossible to predict all use cases. I would only skip the validation of fields that the Scheduler module itself provides.

Yes, I take your point, we should only remove validation from the scheduler fields. Do you know the best way to do this, as the validation for the 'required' fields is done before the scheduler's own hook_node_validate(). Is there a standard pre-validate hook like I've see on date_popup? The nicest way would be to manipulate the form if the user clicked delete and remove the 'required' attributes, but I don't know offhand if that is possible.

Hmm I don't believe it is possible to use #limit_validation_errors on individual elements.

Now that I am thinking about this, we should not mark this field as "required" at all times, it should only be required when the node is not already published. This matches the use case in comment #2.

But this seems to be the case already, I see this line in scheduler_form_alter():

      $publishing_required = variable_get('scheduler_publish_required_' . $form['type']['#value'], 0) == 1 && $node->status == 0;

I just looked through the git history and this has been fixed in commit 2e2f3b3 as part of #1198788: Editing a published node with "Publishing date/time is required" forces users to set a new "Publish on" date/time. I'm marking this issue as a duplicate of that issue. That issue also has some more discussion and advanced use cases on this topic.

Note that if this setting is changed on a site that already has nodes, then this field will be marked as required on unpublished nodes but that is normal, that would also happen if you mark any other field as required using the Field UI module. It is the responsibility of the developer making the change to provide an update hook or script to loop over all nodes and make sure that all fields are valid.

Hi Pieter,

Note that if this setting is changed on a site that already has nodes ... It is the responsibility of the developer making the change to provide an update hook or script to loop over all nodes and make sure that all fields are valid.

I don't think this is a fair comment. I'm sure a large proportion of our users are not coders or developers and would not have the means to write a script or an update hook. They just use Scheduler 'out of the box' and I think it is very likely that sites will have many nodes added, probably before they even install Scheduler, then they set the 'required' option. We need to provide a nicer way for users to delete nodes.

Luckily, I have found another way of doing this, better than before. We can use the #value_callback function on the dates, which gets triggered before field validation. It can check if the button pressed was "delete" and then set #required to FALSE.

In schduler_form_alter, for each of the dates we add:

          '#value_callback' => 'scheduler_date_value_callback',

and then the new function:

/**
 * Callback function for the Scheduler date entry elements.
 */
function scheduler_date_value_callback(&$element, $input = FALSE, &$form_state) {
  // When processing a delete operation the user should not be forced to enter a
  // date. Hence set the scheduler date element's #required attribute to FALSE.
  // Test the input operation against $form_state['values']['delete'] as this
  // will match the value of the Delete button even if translated.
  if (isset($form_state['input']['op']) && isset($form_state['values']['delete']) && $form_state['input']['op'] == $form_state['values']['delete']) {
    $element['#required'] = FALSE;
  }
  // If using date popup then process the callback that would have been done had
  // Scheduler not replaced this with its own one. If using plain text entry
  // then no return value is needed.
  if (_scheduler_use_date_popup()) {
    $return = date_popup_element_value_callback($element, $input, &$form_state);
    return $return;
  }
}

See forms_api_reference.html/7#value_callback for more info on the value_callback function

Patched against 1.1+20

The failed test reported:
Undefined offset: 8192 PHP Notice scheduler.module line 430

Line 430 is

    $return = date_popup_element_value_callback($element, $input, &$form_state);

Do we need to have Date Popup in the test dependencies even if those functions are not actually going to be called during the tests? The current .test file does not do anything with date popup. It runs locally fine and I do not have date_popup in my test dependencies. I even removed the date popup module from my test environment and it still runs ok. Or is it something to do with calling $form_state by reference not by value? Annoying because it all runs perfectly locally.

Changed &$form_state to $form_state in the call to date_popup_element_value_callback()

That's better. I see that Pieter had actually committed a change to .info to add the test dependency on Date before the failed run in #9, so it was nothing to do with that. The call by reference was the problem, and it was wrong anyway, but interestingly it was not highlighted when I ran the tests using Drupal's built in testing on my site. It shows the benefit of also utilising the qa.drupal.org automated tests.

Re-roll was required after 1.1+34

The patch above failed to apply after 1.1+35, so here's a re-roll.

I've written a test. It seems that in the latest 7.x-1.x-dev version the problem only occurs when trying to delete published nodes. Deleting unpublished nodes works fine without the patch.

The "failure" was expected, there is nothing wrong with the code. I tested your tests manually too, all good. Yes I think we have been helped by #1198788: Editing a published node with "Publishing date/time is required" forces users to set a new "Publish on" date/time in the case of unpublished nodes. RTBC

Thanks for the quick review!!

Committed to 7.x-1.x: commit ebfb1bb.

thank you too for all the commits