Updated: Comment #1
Problem/Motivation
The error methods for forms are bad. form_get_errors() uses global state and just returns the errors from the last submitted forms.
This needs cleaning up.
But first, we need better test coverage.
Proposed resolution
Add full test coverage and remove any weirdness that surfaces.
Remaining tasks
Explore possible solutions to removing the global-ness of form errors
User interface changes
N/A
API changes
file_save_upload() needs to be passed a $form_state. In all cases, it was already called from a form's context.
form_*_errors() need to be passed a $form_state. If it is not available, they can still see if there *were* errors by calling \Drupal::formBuilder()->getAnyErrors(), but that returns a Boolean only, not the errors themselves.
Comment | File | Size | Author |
---|---|---|---|
#33 | form-2131851-33.patch | 198.34 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettHere is the first pass. This also adds a getMockForm() method to FormBuilderTest, there were inconsistencies in how the form was mocked.
This gives us 100% coverage of the form methods, and 78.64% overall.
Comment #2
tim.plunkettMuch nicer, IMO
Grr, need to move this back up.
Comment #3
tim.plunkettchx pointed me to #569094: Get rid of form_set_error(), which this might supersede.
This has a lot more changes, which I'll explain, but I wanted to post this before I left work.
Comment #5
tim.plunkettI'm going to hold off on the static stuff, that's easy enough and could be done later.
Fixed a bad search-and-replace.
So, the idea is to work towards making FormBuilder not stateful (storing stuff in instance properties like $this->errors).
To do so, we move it to $form_state. This means that anyone checking for or setting an error needs the $form_state.
Thankfully, in all but 3 places, it is available.
Two are comment_preview() and node_preview(), but those functions are called from a form, and it is trivial to pass in.
The third is _form_set_attributes(), which is called from theme functions and pre_render callbacks, but that has the element, so we can briefly store the errors on there.
That plus the unit tests I wrote, and this is pretty much done!
Unless it fails testing. I have a sneaking suspicion that search.module was doing some weird stuff...
Comment #6
tim.plunkettComment #8
larowlanAdded #1493324: Inline form errors for accessibility and UX to related issues
Comment #9
tim.plunkettHere are some more hacks for the calls that are outside the context of $form_state. We'll need to clean them up at some point, but this is a good start.
Comment #11
tim.plunkettI think I have the next set of fixes figured out, it has to do with errors on nested elements. More later.
Comment #13
tim.plunkettDefinitely a problem with errors on nested elements. Gotta figure this out...
Comment #15
tim.plunkettFound the bug with nested forms, it was a flaw in the original proposal I stole from #569094: Get rid of form_set_error().
Also removed all of my $form_state hacks.
If this passes, I think I'm happy with it.
Comment #16
sunI've to catch up with some recent changes, but overall, this looks good to me.
I like that the function signatures of form_error() and form_set_error() become more or less the same with this.
In fact, after this change has landed, I think we should explore whether we cannot merge the two functions into a single, whereas the first argument can either be a string (denoting the input name) or an array (denoting the #parents). In turn, there'd only be one function for setting errors. (The two functions always confused me.)
There's only one detail that could use some love from my perspective:
Can we find a better name for this method?
It's not a (pure) getter, not a (pure) setter, and it also doesn't really "check" (as in "validate") the form for errors. It would be helpful if the method name would make clear what the function is doing.
Comment #17
tim.plunkettdoCheckErrors() definitely needs a better name. That was the last bug fix, and I was so frustrated/relieved after hours of debugging that I just put it in quickly to ensure it was the right fix.
These are bad changes from the search/replace.
This needs to be rewrapped.
I'll get to this later today, leaving for more reviews in the meantime.
Comment #18
tim.plunkettRenamed to setElementErrorsFromFormState, and fixed the docs issues.
Comment #19
dawehnerWe should open a follow up to always inject the form builder into the form base.
What could this be dropped?
Any reason we drop this here?
Comment #20
tim.plunkett1) I'm thinking we should move the error stuff to its own class, maybe a trait!? and then FormBase can just use it.
Or even if not a trait, it should be a separate interface/class. FormBase doesn't need any of the other methods.
2) Because the previous line has
$form_state = form_state_defaults();
, which is where the errors are stored anyway. This is all because this test calls internal functionality. It should be converted to a unit test eventually.3) Those variables are never used. I have *no idea* why they ever existed.
Comment #21
dawehnerI think it is no problem to call the formBuilder all the time if you need some of those methods.
In that case, remove the @see for this method and the actual method.
Comment #22
tim.plunkettI'll open the followup.
I meant the variable was never used, not the method. It is called via
$this->drupalPostForm('form-test/validate-required-no-title', $edit, 'Submit');
, 3 lines later in the function.Comment #23
dawehnerGot it, thank you.
Comment #24
tim.plunkettI thought about this more, and I think the first step is to separate these methods onto a separate interface. Then when we inject it, we can typehint to that specific interface, and all of the other form building methods will not be available.
It's the difference between
$this->formBuilder->OMG_I_CAN_DO_ANYTHING()
and$this->formErrors->setError()
Ran this by @dawehner and @sun in IRC, @sun and I discussed it and there was no real pushback.
Comment #26
tim.plunkett24: form-2131851-24.patch queued for re-testing.
Comment #27
tim.plunkettRandom fail.
Comment #28
tim.plunkett@webchick suggested that @alexpott review this, but I'm not picky :)
Comment #29
tim.plunkettHere is a version of the patch without all of the additions of $form_state, to better show the actual scope of the issue.
I've also rerolled (with no changes) after the recent core commits.
Comment #31
tim.plunkett29: form-2131851-29.patch queued for re-testing.
Comment #32
tim.plunkettHEAD failed. Retesting this passed.
Comment #33
tim.plunkettRerolled after #2120841: Convert form_options_flatten() to a method on FormBuilder
Comment #35
larowlan33: form-2131851-33.patch queued for re-testing.
Comment #36
larowlanBack again after HEAD issues
Comment #37
webchickalexpott already gave tacit approval of this (tagging as such), and since he's trying to stay focused on CMI I'm OK committing this, esp now that we have some runway before the next alpha. But it's definitely a *huge* API change that will affect basically every module developer who's ported their code to 8.x so far. :( :( Please turn around the change notice as quickly as you can.
One question, doesn't need to hold up commit:
Why the move from OO code to ArrayAPIs here? Shouldn't we be using a method wrapper for setting this like we do for getting?
Committed and pushed to 8.x. Thanks!
Comment #38
tim.plunkettThanks!
Added https://drupal.org/node/2142817
To answer your question, we were previously storing that information in $this->errorSections, which made the FormBuilder class stateful (meaning it tracked something about the form, not just processed it). We moved that information from the FormBuilder class into the $form_state, so that it is tracked by the form, and FormBuilder is one step closer to being stateless.
Comment #39
larowlanChange notice is good, Tim also expanded the earlier FormBuilder change notice to show that form_set_error etc is deprecated (it was missing from there)