By adding #executes_submit_callback to an element a form may be submitted without using a submit button. As a result, $form_state['clicked_button'] might not be set.

scheduler_node_validate() directly checks $form_state['clicked_button']['#value'] without verifying that the index is actually set, causing notices in above scenario.

CommentFileSizeAuthor
#6 2723929-6.check_for_delete_button.patch1.06 KBjonathan1055
#3 check_for_button-scheduler-2723929-3.patch660 byteslegovaer

Comments

ciss created an issue. See original summary.

ciss’s picture

ciss commented
Title: Scheduler assumes that form is submitted via button » Scheduler assumes that node form is submitted via button
legovaer’s picture

legovaer
he/him
Primary language Dutch
Location Duffel
commented
Status: Active » Needs review
StatusFileSize
new check_for_button-scheduler-2723929-3.patch660 bytes

I added a simple check in the if statement that checks wether clicked_button has been set.

jonathan1055’s picture

jonathan1055 commented

Thanks for the patch Legovaer. It removes the error/warning, but we should also think about the implications of using #executes_submit_callback. @ciss I presume you are setting the submit button's #executes_submit_callback to FALSE and then submitting the form in some other way? This patch would mean that you wold not skip over the validation on 'delete' in this case. How exactly are you submitting the form? and does it allow the 'delete' operation?

ciss’s picture

ciss commented

@legovaer I always find checking the submit value to be quite fragile for buttons that use the standard name "op". I'd propose checking for submit callbacks on triggering_element instead:

if(!empty($form_state['triggering_element']['#submit']) && FALSE !== array_search('node_form_delete_submit', $form_state['triggering_element']['#submit'])) {
  // ...
}

@jonathan1055 I'm using a select element that triggers a form rebuild when the selection is changed:

  $layout_element = array(
    '#type' => 'select',
    '#title' => t('Layout'),
    '#options' => $layout_options,
    '#limit_validation_errors' => array(),
    '#required' => true,
    '#empty_option' => '- Select a layout -',
    '#default_value' => $layout_value,
    '#executes_submit_callback' => true,
    '#submit' => array('content_layout_rebuild_submit'),
    '#ajax' => array(
      'callback' => 'content_layout_field_change_layout_js',
      'wrapper' => $field_elements['#wrapper_id'],
      'effect' => 'fade',
    ),
  );

As you can see by looking at #limit_validation_errors the intention isn't even to fully submit the node form.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2723929-6.check_for_delete_button.patch1.06 KB

I'd propose checking for submit callbacks on triggering_element instead

@ciss Thanks for that suggestion, yes it does seem better to check for a programatic value instead of text. Patch for review and testing.

  • jonathan1055 committed c6df094 on 7.x-1.x 
    Issue #2723929 by jonathan1055, ciss: Scheduler assumes that node form...
jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

Avoiding errors with the 'delete' button operation is covered by our simpletests, so that's good. Also manual testing worked fine. There are no other places where the code checks for t('Delete'). Also, this fix is not applicable in 8.x because the node edit form has a link for delete, not a button.

Committed and fixed. Thanks @ciss and @legovaer

Status: Fixed » Closed (fixed)

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