Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
#2239299: Form errors should only be set during validation fixed all known instances of this problem.
But we have no way to enforce it yet.
Proposed resolution
Throw an exception if form errors are set from outside validation (i.e., during submit).
Remaining tasks
Write patch
Write tests
User interface changes
N/A
API changes
If #2239299: Form errors should only be set during validation gets a change record, then this won't need one.
I'd consider *this* the API change, since it's not enforced yet.
Comment | File | Size | Author |
---|---|---|---|
#8 | form-errors-2241735-8.patch | 7.29 KB | tim.plunkett |
#8 | interdiff.txt | 1.39 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettWe have a test, FormValidationTest::testNoDuplicateErrorsForIdenticalForm, which tests that form errors for two forms with the same ID are not displayed visually.
But it doesn't test that the validation of one form doesn't affect the duplicate, because it is still broken.
After discussing with @sun, he found #766146: Support multiple forms with same $form_id on the same page, which exactly describes this problem.
Therefore, the test is only giving us a false sense of security, since it is testing a visual hack, and not a true bug fix.
Comment #2
bleen CreditAttribution: bleen commentedI'm probably just missing something, but why does throwing an exception on bad form error calls mean that we can kill this test?
Comment #3
tim.plunkettThat test fails because of this change.
But the test was implying that #766146: Support multiple forms with same $form_id on the same page was fixed, and it wasn't. So I'm removing the flawed test so that we can go about fixing it correctly.
Comment #4
sunThis looks very nice.
Hm. It looks like must_validate = TRUE can re-enter form validation, even if validation_complete is TRUE.
→ In that edge-case, if a form validation handler tries to set an error, then the form state still believes that validation is complete already + throws an exception.
I think we need to force-reset validation_complete to FALSE when that early-return condition is not entered?
As I wasn't sure what the use-case for must_validate is, I quickly grepped core... and the only usage is in
ViewsExposedForm
— not sure why that forces validation, but let's double-check + verify that resetting validation_complete to FALSE upon re-entering validation does not break anything.I wonder whether the reference to "submission" correctly describes the constraint... how about this:
"Form errors cannot be set after form validation."
Comment #5
tim.plunkettGood points both.
I separated the handling of must_validate from the validation_complete check to make it more explicit, and added a unit test that does expose the bug you described.
Also updated the exception message.
Comment #6
dawehnerThis is not the right exception: " * Exception thrown if a callback refers to an undefined method or if some
* arguments are missing." Maybe invalidArgument of Logic matches better here.
Don't we test some methdo here as well?
I am a bit confused about the removal of the test here.
So we do now loose test coverage that same IDs are not displayed visually now?
Comment #7
sunLogicException
testNoDuplicateErrorsForIdenticalForm()
is a useless test, because it asserts the proper behavior of the bug described in #766146: Support multiple forms with same $form_id on the same pageComment #8
tim.plunkett1 and 2 fixed, 3 ignored again :)
Comment #9
sunThanks! :)
Comment #11
tim.plunkett8: form-errors-2241735-8.patch queued for re-testing.
Comment #12
sunComment #13
sun8: form-errors-2241735-8.patch queued for re-testing.
Comment #14
webchickThis looks like good clean-up. The one area i was concerned with is what would happen now when two forms on the same page had validation errors at the same time, but in manual testing I didn't see any difference before/after the patch.
Committed and pushed to 8.x. Thanks!
Comment #16
tim.plunkettThanks!
Comment #18
Rob230 CreditAttribution: Rob230 commentedI have a multi-part form that gets rebuilt with an internal step variable keeping track of which step of the form you are on.
I get this error -
- after a submit. When it rebuilds the form it seems to revalidate all the data? Is that expected? I would expect the validate and the submit callbacks to be fired exactly once.Edit: never mind, I realised my problem lay elsewhere.