Form validation/submit handlers shouldn't automatically be skipped if button validation/submit handlers exist

Change version so it might get some attention.

Why is there an extra quote on the 4th line?

I had similar issues before. Are you using a form_alter or defining your own form? For testing purposes, you could print_r the contents of all four #validate and #submit arrays after form generation, or use drupalforfirebug and firebug with drupal extension to check the form contents.

It seems I have run into this problem as well (I am on D6.20). I am trying to add mollom support to a contrib module (email, if you must know) and I spent the last few hours trying to figure out why mollom hasn't been kicking in. It seems that the validation functions mollom adds to the form aren't being called, because the form itself adds its validation function to the submit button.

The culprit seems to be line 1051 of forms.inc:

if (isset($form['#validate'])) {
        $form_state['validate_handlers'] = $form['#validate'];
      }

That. combined with this code from form_execute_handlers():

 if (isset($form_state[$type .'_handlers'])) {
    $handlers = $form_state[$type .'_handlers'];
  }
  elseif (isset($form['#'. $type])) {
    $handlers = $form['#'. $type];
  }
  else {
    $handlers = array();
  }

causes the validation functions added to ['#validate'] in the root of the form to be ignored completely.

Please correct me if I'm wrong. I'll suppose I have my patch to email include a move of the validation function to the root of the form, but I guess that really shouldn't be necessary.

I think this is documented behavior (see for example http://drupal.org/node/144132#buttons) and in many cases is what you want. For example, a button that does something tangential to the form's main purpose should not trigger the form's main handlers.

However, this is really confusing for module authors, and all too easy to make a form that accidentally skips running handlers that should have run. I think that for Drupal 8, this behavior should be something you opt-in to only (e.g., via a particular #property you have to set on the button), and otherwise the form-level handlers should run in addition to the button-level ones.

Could someone verify that this is indeed an "acceptable" approach...

You can can pass the $form and $form_state variables through any handler you may still need when creating a special submit handler (changes are because of a second button.)

For example, we have a second user register button (sets role, username and password) however it still needs to pass through the profile_location handler even thought it's a custom separate validation. So...

function MYMODULE_SPECIALUSER_validate($form, &$form_state) {
  // Generate data and manipulate form, ex:
  form_set_value( array('#parents' => array('roles')), array(SPECIAL_ROLE_ID => 'guest'), $form_state);
  // Pass through normal validation
  _user_edit_validate(FALSE, $form_state['values']);
  // Add contrib module validation when routing through this submit handler
  if (module_exists('profile_location')) profile_location_validate_handler($form, $form_state);
}

Seems to work for me.

Might imagine an approach where you look for #validation on the original form and add them dynamically, but I've seen that they are not always present when looking regardless of module weight.

It is indeed documented behaviour, and it's also a pain ;)

The logic for the current behaviour is right here:

    // If the triggering element specifies "button-level" validation and submit
    // handlers to run instead of the default form-level ones, then add those to
    // the form state.
    foreach (array('validate', 'submit') as $type) {
      if (isset($form_state['triggering_element']['#' . $type])) {
        $form_state[$type . '_handlers'] = $form_state['triggering_element']['#' . $type];
      }
    }

We could presumably give module authors the option of whether to execute the form's basic handlers or not.

Like this maybe? (Where my use of += needs checking.)

Marked #1570018: #validate and #submit ignored if button handlers are defined as a duplicate.

Part of me thinks this issue should be "major"... It can be really really confusing and problematic.

> I think that for Drupal 8, this behavior should be something you opt-in to only (e.g., via a particular #property you have to set on the button), and otherwise the form-level handlers should run in addition to the button-level ones.

That makes sense to me. My patch above does the reverse -- executes form-level on request -- so marking as needs work.

Let's make this major -- sounds like people waste a lot of time tracking this down in their forms.

Tagging as Novice too.

Just chipping in to say that yes, this lost me some time today, and although I *now* understand the behavior (and even why it is so), it was not obvious from the #validate documentation.

I didn't go looking for this in the D5-D6 upgrade notes :-}.

I actually don't mind it being by design - as I believe the most common case for specific buttons that have their own #validate handlers is to not worry about the global validation - and ditto for the #submit! - But I'd like to have seen this more clearly announced in the FAPI reference.

I think an opt-in 'event bubble' behavior is probably appropriate here. In the meantime I think I'll just add my global validate function to the per-button #validate array.

Attached the new patch for drupal 8.x.

I am working on extending the test coverage and writing test cases.

Attached simple test cases which will replicate the bug.

These test cases are still failing after applying the patch from #12. I am working on modifying the patch which will pass the test cases.

Nice work pflame, this a good test to show what's happening. If it's not to overwrite the form's submit handler it should be decided which order they get executed. As discussed in IRC, form, then button seems to be reasonable.

Here's a bit of nitpicky things to touch up in the tests. Most relating to coding standards.. Some just tricky to read.

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php
   @@ -244,4 +244,32 @@ function testCustomRequiredError() {
   +
   +  function testFromValidationHandler() {
   

   From should be Form? And could you add a short docblock description above?

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php
   @@ -244,4 +244,32 @@ function testCustomRequiredError() {
   +    // Verify default validate handlers is executed and errors were raised
   ...
   +    // Verify default submit handler will execute if no validation errors
   ...
   +    // Verify that next button validate handler is executed and errors were raised
   

   All comment sentences should end with a period to conform to coding standards. And "handlers is" should be "handlers are".

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php
   @@ -244,4 +244,32 @@ function testCustomRequiredError() {
   +    $this->assertText('Name value: value changed by form_set_value() in form default #validate', 'Form element value in $form_state was altered.');
   ...
   +    $this->assertText('Name value: value changed by form_set_value() in form next button #validate', 'Form element value in $form_state was altered by next submit button.');
   

   "Name value" sounds really weird may want to consider fun and unique name for this field name or just put "The 'name' field's value changed by".

4. +++ b/core/modules/system/tests/modules/form_test/form_test.module
   @@ -2178,3 +2178,65 @@ function form_test_form_state_database_submit($form, &$form_state) {
   +function form_test_form_multisubmit_eles_validate_submit_handlers($form, &$form_state) {
   

   Wow that's a long function name, though i'd suggest not using eles and maybe changing to something like: 'form_test_two_submit_buttons_multiple_handlers', or something just no abbreviations.

5. +++ b/core/modules/system/tests/modules/form_test/form_test.module
   @@ -2178,3 +2178,65 @@ function form_test_form_state_database_submit($form, &$form_state) {
   +    '#value' => 'Save'
   ...
   +    '#submit' => array('form_multisubmit_next_submit')
   

   just needs a comma at the end.

6. +++ b/core/modules/system/tests/modules/form_test/form_test.module
   @@ -2178,3 +2178,65 @@ function form_test_form_state_database_submit($form, &$form_state) {
   \ No newline at end of file
   
   +++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
   @@ -424,3 +424,10 @@ form_test.two_instances:
   \ No newline at end of file
   

   Couple files need a new line at the end to prevent these.

Attached patch has test cases along with the bug fix which will pass the test cases.
Also implemented the feedback provided by joelpittet in the comment #15.

@pflame thanks for the changes! Would you mind posting an interdiff so we can see the changes between #14 and #17? @see https://drupal.org/documentation/git/interdiff

@joelpittet, Thanks for your review. Please check the attached innerdiff file between patch in the comments 14 and 17.

Thanks for the name changes and all the little fixes. Interdiff is really helpful!
Not sure why it's failing so many times though... but I offered a suggestion as well as a few more nitpicks coding standard things.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1411,12 +1411,19 @@ public function doBuildForm($form_id, &$element, &$form_state) {
   +      // If the triggering element specifies "button-level" validation and submit ¶
   +      // handlers to run in addition to or instead of the default form-level ones, ¶
   +      // then add those to the form state.
   

   Watch the 80 character line limit and the ending whitespace on these. Likely your text editor has an option to remove the trailing whitespace and put column rulers on for this.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1411,12 +1411,19 @@ public function doBuildForm($form_id, &$element, &$form_state) {
   +          $form_state[$type . '_handlers'] = array();
   +          // If requested, add the form's base handlers to the list first.
   +          if (!empty($form_state['complete_form']['#'.$type])) {
   +            $form_state[$type . '_handlers'] += $form_state['complete_form']['#' . $type];
   +          }
   +          $form_state[$type . '_handlers'] = array_merge(
   +              $form_state[$type . '_handlers'],
   +              $form_state['triggering_element']['#' . $type]);
   

   An unshift on the original would likely be simpler no? Save a merge and the array union.

3. +++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
   @@ -424,3 +424,10 @@ form_test.two_instances:
   +  requirements:
   +    _access: 'TRUE'
   \ No newline at end of file
   

   Missing newline at the end of the the routing.yml.

This has been fixed elsewhere I think. All forms are objects now, the validate and submit methods are required by interfaces and always called.

It is a default behavior (although not very well documented) for the button's submit/validate handler to skip the form's handlers. This behavior would make sense on a plain button or image button where you'd want to skip main form's validation/submission such as if you just want to publish a node and not save any other fields. But for submit button all other handlers should definitely fire.

For now just merge the form's handlers into your custom submit button:

$form['actions']['save'] = array(
	'#type' => 'submit',
	'#value' => t('Save and continue'),
	'#submit' => array_merge($form['#submit'], array('my_own_submit_handler')),
);