Issue #2561295: One form validation function should be able to bypass other validation steps

added 235 commits

• bafff1a4...d0337fa7 - 233 commits from branch project:11.x
• 0cb13094 - Patch from #43
• 679ee209 - Adopt new convention for mocking element_validate

Compare with previous version

Out of context it's not clear whether this means that validation definitely fails, or whether you could call it without a validation failure and submit the form without running the other validation handlers.

It's not indicating a validation failure, instead it is indicating that no further processing should be done. See example in the other comment.

Yes, but this is still ambiguous. Symfony events for a different example as a stop propogation method - this allows one event to stop other events from running, short-circuiting the chain. However, this doesn't indicate failure or termination, it could be an early 'success' return and the stop propagation is to avoid wasting processing time, not due to security concerns or anything. So I think the docs here need to be more clear that this is a failure indication - i.e. you can't say 'yeah I've done all the validation, no other validation needs to run, go ahead and submit the form'.

changed this line in version 5 of the diff

Given this method is protected, how would this ever get called in this state. If this is to prevent future calls, why not BadMethodCallException instead of the early return?

This method is not to prevent future calls but to prevent further processing of the method. For example, if the CSRF token is missing it is likely a malicious request, so we should stop processing now, before the malicious request causes some other validation function to do something unexpected.

I don't think I worded this very clearly.

From reading the patch, I don't think we ever call ::finalizeValidation() when ::isValidationCanceled() would return TRUE. So if we're never going to hit that early return during normal operatio, the early return appears to be hardening against someone making a mistake later (in a core patch, or a subclass), so why not make it a BadMethodCallException?

If I'm wrong, and we actually do reach this state, it would be good to document the circumstances when that happens.

I agree with you here, I will change this. Thanks.

This throws a BadMethodCallException in the latest version of the code.

@catch I'm not clear on what you are asking me to do, as it seems contradictory to what the other changes in this patch do. \Drupal\Core\Form\FormValidator::validateForm always calls doValidateForm() followed by finalizeValidation(). doValidateForm() is the method that recursively validates the form elements (i.e. by calling itself) and if one of the validations that it runs sets the cancel flag, then it will skip validating anything else and return to validateForm(). finalizeValidation() just prints any error messages that were set to the screen, (see \Drupal\Core\Form\FormErrorHandler::handleFormErrors() ) So, in any case when the form's validation is canceled finalizeValidation() is called. I think maybe what we should do is make finalizaValidation() look like

protected function finalizeValidation(&$form, FormStateInterface &$form_state, $form_id) {
  // Use a service to display error messages attached to the elements that they occurred in.
  $this->formErrorHandler->handleFormErrors($form, $form_state);

  // Mark this form as validated if not cancelled.
  if (!$form_state->isValidationCanceled()) {
    $form_state->setValidationComplete();
  }
}
`

Ahh OK, this makes more sense to me - so we always show the error messages, and then only set validation complete if nothing set it as cancelled. It's easier to read/understand for me this way so looks good! Only comment I would make is I think we can drop 'Use a service to', so just // Display error messages attached to the elements... - I know it already mentions that it uses a service, but I think that's from when services were new.

changed this line in version 8 of the diff

added 249 commits

• 679ee209...80e037e3 - 247 commits from branch project:11.x
• 55e11227 - Patch from #43
• a32a8db8 - Adopt new convention for mocking element_validate

Compare with previous version

added 1 commit

• 24c97849 - finalizeValidation() throws BadMethodCallException instead of returning early

Compare with previous version

added 1 commit

• e395a8cd - Updates ValidationCanceled docblocks in FormStateInterface

Compare with previous version

added 1 commit

• 4b38c2a2 - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements

Compare with previous version

added 148 commits

• 4b38c2a2...e99bc70d - 143 commits from branch project:11.x
• 9b4c8a93 - Patch from #43
• 6c5d83da - Adopt new convention for mocking element_validate
• 88c20273 - finalizeValidation() throws BadMethodCallException instead of returning early
• 4bad0459 - Updates ValidationCanceled docblocks in FormStateInterface
• 1a777daa - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements

Compare with previous version

added 1 commit

• 96575e44 - Removes BadMethodCallException from finalizeValidation

Compare with previous version

added 118 commits

• 96575e44...f9109dc8 - 112 commits from branch project:11.x
• 15aa74a8 - Patch from #43
• f8ea0a6f - Adopt new convention for mocking element_validate
• f28a82e4 - finalizeValidation() throws BadMethodCallException instead of returning early
• 8b2f111c - Updates ValidationCanceled docblocks in FormStateInterface
• 3b6056be - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements
• cd5216ec - Removes BadMethodCallException from finalizeValidation

Compare with previous version

added 9 commits

• cd5216ec...09b18e5e - 3 commits from branch project:11.x
• 8488ddda - Patch from #43
• 19e1f2ac - Adopt new convention for mocking element_validate
• 2ae8098b - finalizeValidation() throws BadMethodCallException instead of returning early
• 9a5c157b - Updates ValidationCanceled docblocks in FormStateInterface
• 3be138f0 - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements
• de9127de - Removes BadMethodCallException from finalizeValidation

Compare with previous version

added 165 commits

• de9127de...27c6d955 - 159 commits from branch project:11.x
• b66655f2 - Patch from #43
• 79bf9afb - Adopt new convention for mocking element_validate
• 261bbcd1 - finalizeValidation() throws BadMethodCallException instead of returning early
• c2ed28d2 - Updates ValidationCanceled docblocks in FormStateInterface
• 4a0c5941 - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements
• eec803ea - Removes BadMethodCallException from finalizeValidation

Compare with previous version

Any reason why the param isn’t strict typed? We can avoid the var casting in the concrete method.

changed this line in version 12 of the diff

Any reason to not strict type this property?

changed this line in version 12 of the diff

Fixed by version 12.

added 1 commit

• f1f09936 - strict types for cancel validation variables

Compare with previous version

added 27075 commits

• f1f09936...349ed0df - 27065 earlier commits
• 97185f6a - Issue #3417721 by catch, kristiaanvandeneynde: Adjust performance tests to avoid random failures
• 1efee51d - Issue #3412464 by mstrelan, smustgrave: Fix strict type errors: Convert...
• 33588d39 - Issue #3406697 by alexpott, chr.fritsch: Fix notice in...
• 28790ce8 - Patch from #43
• b082a31b - Adopt new convention for mocking element_validate
• ff2eefa1 - finalizeValidation() throws BadMethodCallException instead of returning early
• b96a299e - Updates ValidationCanceled docblocks in FormStateInterface
• 7521ad63 - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements
• 9bab456a - Removes BadMethodCallException from finalizeValidation
• 6705a9c9 - strict types for cancel validation variables

Compare with previous version

added 43466 commits

• 6705a9c9...220f04c2 - 43459 commits from branch project:11.x
• c77f73ed - Patch from #43
• 6e5e5e98 - Adopt new convention for mocking element_validate
• 8ef4b697 - finalizeValidation() throws BadMethodCallException instead of returning early
• 3fc49ff8 - Updates ValidationCanceled docblocks in FormStateInterface
• ae4f3175 - Fix non-namespaced classes/interfaces/traits should not be referenced with use statements
• a7e6f64b - Removes BadMethodCallException from finalizeValidation
• 1b386af4 - strict types for cancel validation variables

Compare with previous version

added 1 commit

• a2486fce - strict types in form validators test

Compare with previous version

changed this line in version 16 of the diff