Updated: Comment #38
Problem/Motivation
Looking at validation function now, we have really unfortunate and confusing calls like:
if (!$url_is_valid) {
\Drupal::formBuilder()->setError($element['url'], $form_state, $this->t('The URL %url is not valid.', array('%url' => $element['url']['#value'])));
}
The code to set an error is just manipulating the values of the form state, so there is actually no need do call or inject the form builder.
Proposed resolution
let's make the form state into an object that knows how to set errors and do other internal manipulations so that we can work directly with it in validation functions. We can use the ArrayObject interface to simplify backward compatibility and conversion.
if (!$url_is_valid) {
$form_state->setError($element['url'], $this->t('The URL %url is not valid.', array('%url' => $element['url']['#value']));
}
Remaining tasks
todo
User interface changes
N/A
API changes
If FormBuilderInterface::buildForm() is called directly, it must be an instance of FormStateInterface.
Before:
$form_state = array(
'rebuild' => TRUE;
);
$form_builder->buildForm('Drupal\my_module\Form\MyForm', $form_state);
After:
use Drupal\Core\Form\FormInterface;
$form_state = new FormState(array(
'rebuild' => TRUE;
));
$form_builder->buildForm('Drupal\my_module\Form\MyForm', $form_state);
Because FormState uses ArrayAccess, you can still access properties as an array, but the method is preferred:
Before:
$form_state['rebuild'] = TRUE;
After:
$form_state->set('rebuild', TRUE);
In order to do things like setting errors, you no longer need the whole form builder:
Before:
\Drupal::formBuilder()->setError($element, $form_state, $message);
After:
$form_state->setError($element, $message);
The following interface methods have been changed from array &$form_state
to FormStateInterface $form_state
:
FormInterface::buildForm()
FormInterface::validateForm()
FormInterface::submitForm()
EntityFormDisplayInterface::buildForm()
EntityFormDisplayInterface::validateFormValues()
EntityFormDisplayInterface::extractFormValues()
FieldItemInterface::settingsForm()
FieldItemInterface::instanceSettingsForm()
FieldItemListInterface::defaultValuesForm()
FieldItemListInterface::defaultValuesFormValidate()
FieldItemListInterface::defaultValuesFormSubmit()
FormatterInterface::settingsForm()
WidgetBaseInterface::form()
WidgetBaseInterface::extractFormValues()
WidgetBaseInterface::flagErrors()
WidgetBaseInterface::getWidgetState()
WidgetBaseInterface::setWidgetState()
WidgetInterface::settingsForm()
WidgetInterface::formElement()
WidgetInterface::errorElement()
WidgetInterface::massageFormValues()
FormBuilderInterface::buildForm()
FormBuilderInterface::submitForm()
ImageToolkitInterface::settingsFormSubmit()
MenuLinkFormInterface::extractFormValues()
PluginFormInterface::buildConfigurationForm()
PluginFormInterface::validateConfigurationForm()
PluginFormInterface::submitConfigurationForm()
BlockPluginInterface::blockForm()
BlockPluginInterface::blockValidate()
BlockPluginInterface::blockSubmit()
BookManagerInterface::addFormElements()
CKEditorPluginConfigurableInterface::settingsForm()
ContentTranslationHandlerInterface::entityFormAlter()
EditorPluginInterface::settingsForm()
EditorPluginInterface::settingsFormValidate()
EditorPluginInterface::settingsFormSubmit()
SelectionInterface::validateAutocompleteInput()
FilterInterface::settingsForm()
SearchInterface::searchFormAlter()
WizardInterface::buildForm()
WizardInterface::validateView()
WizardInterface::createView()
ViewsFormInterface::getFormState()
Comment | File | Size | Author |
---|---|---|---|
#65 | form_state-2225353-65.patch | 1.12 MB | tim.plunkett |
#64 | interdiff.txt | 599 bytes | tim.plunkett |
#64 | form_state-2225353-64.patch | 1.13 MB | tim.plunkett |
Comments
Comment #1
dawehnerComment #2
tim.plunkettI wish you luck, sir. I tried really hard to do this when I was working on the initial FormBuilder code.
Consider code like this:
$form_state does not exist, and is initialized as an array.
Unless every entry point into FAPI code has code like
, I don't see how else to avoid this.
Also consider that even if you try to use ArrayObject or whatever, it will not satisfy the typehint
array $form_state
, no matter how hard you try.Comment #3
tim.plunkettI should clarify. I think this would be wonderful for DX. There are so many weird parts of $form_state (look at FormBuilder getUncacheableKeys() and getFormStateDefaults()) that have hard-to-find documentation, and making these methods on a class would be really awesome. I just couldn't get it to work, and didn't want to mess with every array typehint in existence.
Comment #4
larowlanShouldn't it be FormElement->setError where SelectElement et al extend FormElement :)
Comment #5
tim.plunkett#4:
a) OO-ifying the form elements is D9 territory, and should explore symfony forms
b) The errors are canonically stored in the form_state, not on the element themselves.
Comment #6
pwolanin CreditAttribution: pwolanin commentedThanks for the warnings of dragons - I also suggested a trait on the for class, but that would be a lot of code duplicate under the covers.
I'll tackle other more obvious issues first if this on is such a rathole.
Comment #7
tim.plunkettSo on a whim, I'm trying this again.
Comment #8
tim.plunkettThis is going to be large.
This doesn't even provide any useful methods yet, just the necessary conversions.
Comment #11
tim.plunkettRerolled after #2268761: Remove support for function-based forms, and fixed installer.
Comment #13
tim.plunkettMissed a spot.
Before patch
With patch so far
I got all of the ones in interfaces, all of the methods should happen, not as worried about form_alter yet.
Comment #15
tim.plunkettNot pictured in the interdiff: All of &$form_state is now typehinted
Comment #17
tim.plunkettComment #19
tim.plunkettComment #20
tim.plunkettOkay, here are some example methods, with one example each.
I also deprecated FormErrorInterface.
I would not like to expand the scope past this point at all. The typehint renames already make this patch monstrously huge :(
Comment #22
tim.plunkett:)
Comment #23
tim.plunkettFixed all known docblocks to stop calling form_state an array.
Comment #24
dawehnerReviewing the file using whatthepatch, see http://nbviewer.ipython.org/9559bcc48fccbf49c265
Unneeded reference.
Missing changes of actual code.
references, more in other api.php files
This is also in other files, pretty much everywhere
This should be explained in the issue summary or somewhere.
Comment #25
tim.plunkettI'm leaving the references on $form_state for all of FormBuilder, FormValidator, and FormSubmitter. They need to physically replace the $form_state, which can only be done if it is passed by reference. The same thing goes for views_ajax_form_wrapper().
I removed the rest of the references.
I need to explain the change to EntityFormBuilder::getForm() in the issue summary, but I have to run for now.
Comment #27
dawehnerContinued review.
We should try to have the same parameter names here.
This is such a great change!
It would be great to also use $form_state->get('values') here or maybe getValues() (not sure whether there is a specific method for that).
Can we have a @return $this just for consistency on all set*() methods?
It would be great to document here, why we need a clone here.
Let's be honest, this is not enough. We could add some @see when the form_state is created and processed for example. Everyone
will use that interface
Lazyness is flying around here. Can we also use a better name instead of form_state_additions? We don't really add something here.
Did we considered to merge the arrays?
Did we considered to just store a property?
Is this code which will be kept in the future?
we could consider to use the isset optimization here.
Comment #28
dawehnerThis is an API change which will affect everyone, so adding yet another tag.
Comment #29
pounardI didn't read the full thread, but I think this is an extremely good idea to move the form API toward being more maintainable. In the long term (I don't know if you planned it or not once conversion is done in every form) I think that technical properties of the form state should become object properties with default values set to make it even more comprehensible, and you would not need the addFormStateDefaults(), getUncacheableKeys() and getCacheableArray() methods anymore. Values should be an object of its own, as well as storage: doing that you could return easily storage or values without need to specify the & in front of the get() method which can have weird side effects on object internal state. And you could just cache those two without having to check for (un|)cacheable keys.
Aside of that, that's great work you're doing!
Comment #30
tim.plunkettIdeally yes, but I think that can happen after this conversion goes in.
I addressed some of #28 but not all of it, I just wanted to get this applying again. I'll have to keep rerolling until beta blockers go in, I'll have more time for this Sunday night and then next week.
Comment #33
tim.plunkettRandom fail. Retested.
Comment #34
tim.plunkettI wanted to look at #29 more, but I hit an issue with the Views UI. Splitting off to #2307885: Refactor FormState to use dedicated properties instead of an associative array.
Here's a reroll.
Comment #35
tim.plunkettComment #37
tim.plunkettFixed code added in #2305293: Wrong #states for "Restrict to the selected roles" in views ui
Comment #38
tim.plunkettTrying to remove the clone mentioned in #27, and also moving setValue.
Also updated the issue summary.
We have WAY too many interfaces that deal with plugins :(
Comment #42
tim.plunkettSo much for that!
Comment #44
tim.plunkettOh, whoops, it seems it was the setValue() change?! Sorry for the noise.
Removed that addition.
Comment #45
tim.plunkettAdded more docs, moved tests to the right place, renamed some internal properties.
Comment #47
tim.plunkettWhew, got that all figured out.
Comment #48
tim.plunkettOkay, ready for more reviews, I'm happy with this one.
Here is an attempt to remove the typehint and use statements from the patch, highlighting just the functional changes.
Comment #49
tim.plunkettBased on some IRC conversations, I went ahead and added methods we're likely going to need.
Code is in my sandbox: http://cgit.drupalcode.org/sandbox-tim.plunkett-1698392/log/?h=2225353-form
Comment #51
dawehnerI really love this patch, reviewed #48
Are we sure we want to not explicit typehint it to array? I know you are not a fan of it, but it helps people to know what they should put in.
Is it just me that there is no point in having this method? Is this done to have a small patch? If yes, feel free to open up a followup.
Do we have a follow up what to do with this special snowflake in views?
This is just awesome!
Comment #52
tim.plunkett#51.1 That's a fine suggestion.
#51.2 Correct, it's just used way more than you'd think. Opened #2308821: Replace FormErrorInterface with $form_state->setErrorByName() and $form_state->setError()
#51.3 Honestly this doesn't really bother me. I don't think it's any worse now than it was before.
#51.4 <3 unit tests
I forgot a key line in setValue(), that should fix the installation.
Comment #53
dawehnerI think this is RTBC
Comment #54
tim.plunkettJust to note, since this conflicts with two beta blockers, I asked @dawehner to postpone this instead of actually RTBCing.
I'll be posting rerolls of this after each of those go in, leaving postponed for now.
Comment #55
YesCT CreditAttribution: YesCT commentedthis is gonna be great to be able to use!
------------
i. note 52 applied on 5776d98
ii. I looked up this while reading the patch
start a PHP function with an ampersand?
http://stackoverflow.com/questions/1676897/what-does-it-mean-to-start-a-...
iii. I only read carefully the first half of #48 (up through the FormStateInterface)
iv. I would have rerolled this and just fixed some of my nits, but it's assigned and work is in a sandbox... and didn't want to disturb work that might already be in progress.
v. some of these might be dismissed with "we are not doing that" or "that will be done in issue XX"
why did this take out the array type hint? can we add that back in?
a. We have more verbose @deprecated info template now.
https://www.drupal.org/node/1354#deprecated
Something like
b. Does this actual replace all the usages of setFormError? I dont think so based on the comments so far, so we need a follow-up issue.. which we have: #2308821: Replace FormErrorInterface with $form_state->setErrorByName() and $form_state->setError() Ok.
a. need a child issue for this and reference the url in the @todo.
b. OH! would this be the issue to actually use the methods everywhere?
a. is args also optional?
b. Does it help to say an array of arguments instead of list?
should the example $form_state['rebuild'] be $form_state->rebuild ? or $form_state->setRebuild()?
similar question here if the doc should say: If you set $form_state->redirect, it will... (or the redirect methods)
nit. double space "be a string"
a. these property names have to stay redirect_route and not be renamed redirectRoute since the usages are still going to be $form_state['redirect_route'] and not converted to $form_state->set('redirect_route') ... uh, $form_state->setRebuild() ?
b. do we want a follow-up to convert array accesses to method calls like that later?
c. do we want a follow-up to rename the properties to lowerCamelCase?
nit. IDs (not ids). But this doc is just being moved... so maybe not worry about it.
a. this can be wrapped for 80 chars.
b. can we provide an example?
c. should other array() properties also provide examples, or document their keys and values? or can we @see some set method that lists out what keys are in the array?
nit. missing one line summary here. Maybe just:
Constructs a \Drupal\Core\Form\FormState object.
should we name this getOffset()?
a. why doesn't this return $this? We wont need to chain this one?
b. should we name this setOffset() ?
similarly, unsetOffset() ?
is there already an issue we can link to for this @todo? Maybe #2189661: Replace $form_state['redirect_route'] with setRedirect() ?
nit, standards thing,
https://www.drupal.org/node/1354#lists
the items in a list following : should not be indented.
gets/returns a *reference* to any arbitrary property?
Stopped after the interface.
Comment #56
tim.plunkettThanks for not rerollinf, I'm keeping it fresh in the sandbox. I'll address the rest of your comments later today
Comment #57
tim.plunkettKeep in mind this issue is RTBC and is just waiting for the conflicting beta blocker to go in.
That said, it doesn't hurt to have more in-code issue links for @todos.
Will mark postponed again, just want to get a new test run.
#55.1 This was done in #52.
#55.2 We don't know when this will be removed, I'm not okay with promising that.
#55.3 #2310255: [meta] Remove ArrayAccess from FormState, added in code reference
#55.4 These docs are copy/paste from the old ones.
#55.5 These docs are copy/paste from the old ones.
#55.6 These docs are copy/paste from the old ones.
#55.7 These docs are copy/paste from the old ones.
#55.8a Yes
#55.8b See 55.3
#55.8c It might happen as part of the other issue, but I'm not worried about it either way
#55.9 These docs are copy/paste from the old ones.
#55.10 These docs are copy/paste from the old ones.
#55.11 Sure
#55.12 This is a method from ArrayAccess, we have no choice.
#55.13 This is a method from ArrayAccess, we have no choice.
#55.14 This is a method from ArrayAccess, we have no choice.
#55.15 Sure
#55.16 Fixed
Comment #58
tim.plunkettGreat!
Comment #59
tim.plunkettAdded a draft change notice: https://www.drupal.org/node/2310411
Comment #60
jibranI think we should add all the methods of
\Drupal\Core\Form\FormStateInterface.
to the change notice..
Comment #61
tim.plunkettI updated the draft with a link to where FormStateInterface will live on api.d.o, but no we're not replicating a list of all the methods on the change notice. We don't do that in other change notices.
Comment #62
tim.plunkettThe blocker went in. No other changes since RTBC.
Comment #64
tim.plunkettWeird, must have been a bad rebase.
Comment #65
tim.plunkettRerolled for #2301319: MenuLinkNG part5: Remove dead code; and party!
Comment #66
Dries CreditAttribution: Dries commentedI couldn't do a full review over this patch but like it directionally. It seems like the right thing to do. It most likely needs a re-roll after the menu link commit.
Comment #67
tim.plunkettWe cross-posted; #65 is the reroll after all of the menu link patches. This is ready!
Comment #68
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #70
jhodgdonHey, great job at fixing up the documentation in conjunction with this patch -- much appreciated! I filed a small followup:
#2313089: Form generation topic needs quick follow-up fix
It's a novice issue; I expect it will be snapped up by one of our great novice patchers before too long and fixed.
Comment #71
tim.plunkettThanks!
Comment #72
jhodgdonI just filed a near-critical documentation follow-up on this:
#2313603: FormStateInterface::getValues() says "sanitized" -- vague and misleading
It would be great if someone who worked on this issue could add a note there as to what should be done, then add tag "Novice" so someone will quickly make a patch.
Comment #73
tim.plunkettThat text was added on Mon May 14 13:43:38 2007, in #138706: FormAPI 3: Ready to rock.