One form validation function should be able to cancel other validation steps

As this would still make sense for modules like CAPTCHA ( ✨ [PP-1][2.x] Do not execute other form validations if CAPTCHA is wrong Postponed ) or any other modules like honeypot, that should definitely run their validation before other requests are made, does it make sense to reroll the patch for Drupal 10.1.x?

I guess it would be helpful to have a core maintainer discussion about this first before investing further time here?

See the linked issues for a real-world benefit and use case.

Should it be something that each form validator could control? e.g.

+    // Allow a validator to stop processing other validators if it hits a
+    // critical problem.
+    $continue_validators = TRUE;
     foreach ($handlers as $callback) {
-      call_user_func_array($form_state->prepareCallback($callback), [&$form, &$form_state]);
+      if (!$continue_validators && !$form_state->hasAnyErrors()) {
+        $continue_validators = call_user_func_array($form_state->prepareCallback($callback), [&$form, &$form_state]) ?? TRUE;
+      }
     }

Or we use different types of errors or flags for the errors in the validation callback?

I guess in most cases the error in validation should determine if other validations should happen or not. $form_state might be the right place for that and nice DX?

Something like:

/**
 * {@inheritdoc}
 */
public function validateForm(array &$form, FormStateInterface $form_state) {
  if (strlen($form_state->getValue('phone_number')) < 3) {
    $form_state->setCriticalErrorByName('phone_number', $this->t('The phone number is too short. Please enter a full phone number.'));
  }
}

Note: $form_state->setCriticalErrorByName(); (+ Critical)

or

/**
 * {@inheritdoc}
 */
public function validateForm(array &$form, FormStateInterface $form_state) {
  if (strlen($form_state->getValue('phone_number')) < 3) {
    $form_state->setErrorByName('phone_number', $this->t('The phone number is too short. Please enter a full phone number.'));

    // This is a critical error, validation should not proceed with this error:
    $form_state->cancelValidation();
  }
}

Note: $form_state->cancelValidation();

Just an idea...

Anybody, I like your suggestion of cancelValidation(), I tried implementing it in this patch. Since it is so different from the previous patches, I just wrote it from scratch, so there is no interdiff. (I created this patch on 9.5 because that's how my test environment was setup at the time). The only place there might be a problem (the patch may not apply) is in the test, if that's the case, I'll fix it in short order.

Patch applied successfully, but test bot had opinions on spelling and indentation. Here's a new patch generated against 10.1 this time.

Thank you @AlexGreen, that's looking good! :)
What do @tim.plunkett, @David_Rothstein, @DamienMcKenna and the others think about this way? Would be nice to get feedback to be able to proceed here. Who else should be involved?

Tagging for subsystem maintainers review to answer #35

+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -109,8 +112,7 @@ class FormValidator implements FormValidatorInterface {
-        $this->finalizeValidation($form, $form_state, $form_id);
-        return;
+        $this->cancelValidation($form, $form_state, $form_id);

This change (and the non-fail-ing-ness of the patch) mean that we're missing test coverage. Because now nothing is calling finalizeValidation(), which means nothing is calling setValidationComplete(), which means that $form_state->isValidationComplete() will start returning FALSE

I have no recollection of this issue, but apparently I wrote a patch for this 8 years ago? Cool.

@tim.plunkett Is this what you meant?

+++ b/core/tests/Drupal/Tests/Core/Form/FormValidatorTest.php
@@ -132,7 +132,7 @@ class FormValidatorTest extends UnitTestCase {
-    $this->assertFalse($form_state->isValidationComplete());
+    $this->assertTrue($form_state->isValidationComplete());

@@ -407,7 +407,7 @@ class FormValidatorTest extends UnitTestCase {
-    $this->assertFalse($form_state->isValidationComplete());
+    $this->assertTrue($form_state->isValidationComplete());

Oh yeah, the interdiff shows it clearly. With that last fix, we're no longer changing existing behavior 👍🏻

Real nitpicky but can the new functions have a typehint return?

Also think it would benefit from a change record for the new functions. Maybe an example of how they can be used.

I'll write a change record for this.

Here is a draft change record, any suggestions/reviews welcome https://www.drupal.org/node/3360541 →

@smustgrave Here is a new patch with the return typehints.

Running PHPStan on *all* files.
 ------ ---------------------------------------------------------------- 
  Line   core/tests/Drupal/Tests/Core/Form/FormValidatorTest.php         
 ------ ---------------------------------------------------------------- 
  390    Trying to mock an undefined method element_validate() on class  
         stdClass.                                                       
 ------ ---------------------------------------------------------------- 

Not exactly sure how to fix this error, it was working in #38 and I didn't change this code. @smustgrave

Reran #38 and see the same. Most likely another ticket was merged and will have to update this one. Should see if that method is on the class being mocked anymore or if it moved

May have some time tomorrow to help take a look.

📌 Only mock methods defined in interfaces Fixed changed the existing usages of stdClass in this test, changing this one to match.

And switch to an MR.

Updated the CR to be introduced in 10.2.

I've rebased 561295-one-form-validation to 11.x so it is mergeable again.

Left some comments on the MR.

Apart from those, I'm slightly concerned about the 'canceled' naming. Canceled usually means something is called off, but this is short circuiting/finishing early. The word that came to mind is 'terminate' i.e. setValidationTerminated() etc. Or even just 'finished' like 'setValidationFinished()'?

@catch In this case, we think "terminated" is the closer concept that this change is intending to signify. We could change it to setValidationTerminated() if that's desired.

For the open threads. Seems at minimum comments were requested to be updated.

@catch I have left a comment on the merge request regarding the question raised in your review.

Replied in the MR - my initial review was completely wrong, but the change you suggested makes it a lot more obvious what's happening and should prevent others from getting confused too, so looks great.

This patch allows validators to cancel validation and I think is a good improvement. However, it would be great to be able to prioritize validations as said in the issue report. So, for example, taking example 2, Clam AV scan would have a low priority so an uploaded file is only scanned if everything else is valid. Or let's think on a form with some kind of authorization field and some other fields: if the authorization field is wrong the other fields are not validated, avoiding information leaking about those fields.

Currently, validators are run from bottom to top while traversing the form array. We could just collect them and run in the given priority. The format of the validation callback would change form just a callback name to an object with the callback and the priority.

Currently, you can only prioritize validators in the same level (or form elem: the root elem or any single elem) but not between them.

With priorities this improvement would be even more useful. Should I create a follow up about this? Discuss it somewhere?

@tunic a follow-up for that sounds good.

Follow-up added!

✨ Add priority to form validators Active

Ready for re-review @catch

Seems that all feedback has been addressed.

Looking at the CR still seems relevant, but could use a 2nd look over.

The failure seems random but I've reran multiple times so wonder if HEAD is broken.

Tests are passing, back to RTBC

2 questions in MR

I'm triaging RTBC issues → . It is always nice to see older issues getting close to commit. Thanks!

I read the IS, skimmed through the comments and the MR, and read the change record. I didn't find any unanswered questions.

I do see that the issue summary has not been updated since 2015. And I think the proposed resolution is out of date. That should be up to date to help reviewers of this issue as well as anyone who winds up here doing any research.

In #62 @smustgrave, thinks the change record is relevant but wants another set of eyes on it. That has not happened. I am excluding myself from that because I am not familiar with the issue. I will add that I think it should begin with a sentence or two to explain the value of this change. Or to answer the question, why, as a developer would I want to use this feature?

Due to the importance of an up to date and accurate proposed resolution section in the Issue Summary I am setting this to Needs work. Also, someone else should check the change record. Fortunately, those steps should be straightforward.

Fixed strict typing and updated the proposed resolution to match how we actually resolved the issue.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Issue reported by the bot has been corrected, so moving to Needs Review again.

smustgrave → changed the visibility of the branch 11.x to hidden.

Would this count as an API change? I feel yes but not familiar with the issue to say. But should be added to issue summary.

Updated summary with three public functions that are API changes in the current MR.

I also think this would need a change record.

There's already a change record at https://www.drupal.org/node/3360541 → , it would be useful if people could review it to make sure it's up to date with the current state of the MR though.

Ops, I missed that.

I've reviewed the change record and the MR changes and the change record seems up to date to me.

Believe everything is in order. Believe this is good.

Resolved my own thread and applied the suggestion :void to the test.

Needs work for @nod_'s comment on the MR.

Also - if you cancel validation and there are no errors set yet should the form submit? I don't think it should. I read the code and thought it might not submit - but after re-reading I think it will and I think this is wrong and potentially ends up with way worse security issues.

Also I think we should consider changing the name from cancel validation to halt validation in light of the behaviour change I'm suggesting above.

I agree with @alexpott, if the form cancels validation and there are no errors it seems submit handlers will be run.

This is because FormBuilder::processForm runs submit handlers if form has no errors and is not rebuilding, see line 595:

// If there are no errors and the form is not rebuilding, submit the form.
if (!$form_state->isRebuilding() && !FormState::hasAnyErrors()) {
  $submit_response = $this->formSubmitter->doSubmitForm($form, $form_state);

FormState::hasAnyErrors uses internally a static variable in FormState, $anyErrors. I'm not sure why a static variable is used (maybe because subforms?), but anyway FormState::setAnyErrors is not called anywhere in the MR so that's why I think submit handlers will run.

I think the problem lies in FormBuilder::processForm. It should only run submit handlers if form is not rebuilding AND validation has completed without errors, not just no errors were found.

Either FormBuilder::processForm checks also FormState::isValidationCanceled or FormState provides a way to check validation was completed without errors in a new method. The first approach is easier, but I think is FormState who should be in charge of deciding if the form was completely validated instead of requiring two calls (logic should be inside FormState, not force users of FormState to remember to perform these two checks). However, because the error check uses this static method and an internal static variable, I'm not sure how to do it because I don't know why a static variable is used in the first place.

Also, the name change proposed is a good idea, it's clearer and describes more accurately the proposed behavior.

@alexpott, @tunic, thank you for the feedback.

It sounds like are suggesting the following path forward... do you agree? Anything that I missed?

1. Change substr($callback, 0, 2) == '::' back to str_starts_with($callback, '::') as per @nod_'s suggestion
2. Change cancel and canceled to halt and halted respectively
3. Find some way to abort submitting the form or otherwise prevent running submit handlers when the form state reaches the "halted" state (previously "cancelled"), possibly by changing FormState::setAnyErrors
4. Investigate where FormState::setAnyErrors is used, and/or git history to see if we can figure out when/why it was made static in the first place.
5. See how easy it would be to create a new function in FormState to check if validation was completed without errors, and get other code to use it?

@AleexGreen, from what I've understood and from what I've commented I think you have summed up the current state of the issue perfectly.

Thanks!

4. Investigate where FormState::setAnyErrors is used, and/or git history to see if we can figure out when/why it was made static in the first place.

FormState::setAnyErrors and FormState::hasAnyErrors were created in #2225353: Convert $form_state to an object and provide methods like setError() → between comments 19 and 20 by @tim.plunkett in July 2014 for 8.0.x (i.e.: before a Drupal 8.0 release)
No explanation given for why they were made static.

According to Sling Academy guide on PHP static properties

When you declare class properties […] as static, they are accessible without needing to create an instance of the class. This feature is useful for values […] that are logically intrinsic to the class rather than to any individual object

Maybe @tim.plunkett intended for “protected static $anyErrors” to be a “flag” that lasts for the duration of the current page request for the current user? (regarding subforms, inline_entity_form existed for D7 at that point, but it’s unclear if that was considered the use-case).

public static function setAnyErrors() is used in public function setErrorByName() after recording the element with the error and the error message. It’s also used in public function clearErrors() but isn’t defined in an Interface.
public static function hasAnyErrors() is defined in FormStateInterface, and is used ~11 times across Drupal core (as of commit bcabbbdb75 to 11.x on 2024-06-03)

1. 3 times in \Drupal\media_library\Form\AddFormBase and 3 times in \Drupal\Core\Form\FormBuilder to abort processing
2. In \Drupal\Core\Ajax\AjaxFormHelperTrait::ajaxSubmit() to output status messages in an AJAX response
3. In \Drupal\media_library\Form\FileUploadForm::validateUploadElement to abort processing and force the user to re-upload a file
4. In \Drupal\Core\Form\FormStateDecoratorBase::hasAnyErrors() to be a backend for its own \Drupal\Core\Form\FormStateDecoratorBase::hasAnyErrors() function
5. In \Drupal\Tests\Core\Form\FormStateTest::testSetErrorByName() to test setErrorByName()
6. In \Drupal\form_test\Form\FormTestStoragePageCacheForm::validateForm() to know when to cache the form

Couldn’t decide if it made more sense to throw an exception in\Drupal\Core\Form\FormSubmitter::executeSubmitHandlers or simply return early.

If we throw an exception, then anything calling FormBuilder::executeSubmitHandlers will have to catch the exception, and if nothing catches it then the user will get a WSOD (i.e.: possibly from an expired form token because they left the page open in their browser for too long) which seems like a bad user experience

If we return early, then anything calling FormBuilder::executeSubmitHandlers doesn’t really know that something went wrong (i.e.: the node didn't save)

P.S. latest commit didn't include tests, was looking for feedback. (Test fails seem to be unrelated)

What is the best path to take, throw an exception or return early?

I would say throwing an exception.

I've searched in one of my projects and only found a few references to executeSubmitHandlers in core, and none on the modules folder (and there are plenty of modules in this project), so it seems it is a very internal function that is not commonly called.

Maybe some modules are impacted, but I guess they will not many, and throwing an exception sounds like the right way.

Not sure what this tag was.

Checking MR and appears to be throwing errors.