| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | critical |
| Assigned: | yched |
| Status: | closed (fixed) |
| Issue tags: | API change, D7 Form API challenge |
Issue Summary
Field API currently stores meta information about the fields being edited in $form['#fields']. For each field, $form['#fields'][$field_name] has:
- 'field', 'instance': the full $field and $instance definitions.
This is the information that all widget processing callbacks (FAPI process, validate, value callbacks) are supposed to use if they need some info about the field or instance - NOT field_info_field() or field_info_instance(). This lets us build form elements for 'variants' of a field, different from the officially stored definition - for instance in views exposed filters, or in Field UI admin screens.
- 'form_path': path to the field form element within the $form structure (what Form API actually calls #array_parents).
This is used in the AJAX 'add more' handler, and in the assignment of field validation errors down to actual form elements.
This information clutters the $form array, and makes a print_r($form) unreadable. This really belongs in $form_state.
We can do that now that #634440: Remove auto-rebuilding magic for $form_state['storage'] is in.
What the patch does:
- removes $form['#fields'] and introduces $form_state['storage']['field_info'] - for each field:
$form_state['storage']['field_info'][$field_name][$language] = array(
'field' => $field definition,
'instance' => $instance definition,
'array_parents' => (previoulsy 'form_path') FAPI #array_parents : path to the field elements in $form
'parents' => = FAPI #parents : path to the field values in $form_state['values']
'errors' => the array of field-level errors reported by hook_field_validate()
); - fixes widgets that were incorrectly relying on field_info_[field|instance]() instead of $form['#fields'], and also clearly documents that practice in hook_field_widget() (actually that's still a @todo in this version of the patch)
- makes field_add_more_js() and field_default_form_errors() use the $form_state 'array_parents' info (prevsiously used the info from $form['#fields']).
- makes field_default_extract_form_values() use the $form_state 'parents' info instead of assuming values are in $form_state['values'][$field_name][$language]
- automatically populates 'parents' and 'array_parents' at #after_build time.
- adds $form and $form_state args to hook_field_widget_error(). That's a widget hook, those arguments belong here, even if most implementations probably won't need them (current ones in core do not). API addition rather that API change, since nothing breaks if a given hook overlooks those additional args.
Worthy gains, aside from uncluttering $form:
- This ensures that Field API form features (add more, error reporting, saving field values) do not break when some hook_form_alter() changes the form structure, or when some field widget is stuck at a random place in a random form. For instance, this should allow an 'add more' button in the 'default value' widget in Field UI.
- $form_state['storage']['field_info'] being keyed by langcode, this is also an important step to allow forms with multiple languages ('side by side translate' in contrib...)
Patch still has a few documentation @todos, posting it here for peer and bot review.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| field_form_state.patch | 31.82 KB | Idle | Failed on MySQL 5.0 ISAM, with: 15,256 pass(es), 22 fail(s), and 144 exception(es). | View details |
Comments
#1
Minor FAPI bug I found out along the way: #641544: FAPI #type 'container' ids generated twice
+ forgot to set to CNR
#2
uh oh. Using #after_build to shift #parents and #array_parents into $form_state...? That, we need to think about.
#3
Also linking back to the dupe #627730: Make field forms resilient to form_alter() structure changes, which identified the #parents issue in fieldable entity forms in the first place.
#4
The last submitted patch failed testing.
#5
Argh @ translation tests.
Well, for now, reroll after #619230: Update 'default field value' UI code for D7 Field API and fix for the 'Field attach' failure.
#6
The last submitted patch failed testing.
#7
Attached patch should fix the TF failures.
#8
OK, reducing the scope a little.
Attached patch removes the #parents part - automatic extraction of #parents at after_build time, + using the 'parents' in field_default_extract_form_values().
I still think it's a good approach, but:
- this can be debated in #627730: Make field forms resilient to form_alter() structure changes,
- there are other places in core that currently rely on a hardcoded location in
$form_state['values'][$field_name][$langcode]: file_field_widget(), locale_field_node_form_update_field_language(), and changing those would take the patch too far off,- I don't want this to stop the $form['#fields'] -> $form_state['storage']['field_info'] change from getting in.
So, updated patch does as described in the OP without item 4 and the 'parents' component in $form_state['storage']['field_info'].
+ Marking as critical, since with the current code, a contrib module form_altering the structure of the $form without updating $form['#fields']'s 'form_path' accordingly causes field-level validation errors not to be reported on the form.
#9
The last submitted patch failed testing.
#10
yched requested that failed test be re-tested.
#11
Rerolled after #639428: Remove #process pattern from text field (no more changes needed in text.module)
#12
Rerolled after #635094: Unify "language neutral" language codes
#13
easier tracking
#14
Rerolled.
#15
Rerolled.
Patch summary, updated from the OP:
Field API currently stores meta information about the fields being edited in $form['#fields']. For each field, $form['#fields'][$field_name] has:
- 'field', 'instance': the full $field and $instance definitions.
This is the information that all widget processing callbacks (FAPI process, validate, value callbacks) are supposed to use if they need some info about the field or instance - NOT field_info_field() or field_info_instance(). This lets us build form elements for 'variants' of a field, different from the officially stored definition - for instance in views exposed filters, or in Field UI admin screens.
- 'form_path': path to the field form element within the $form structure (what Form API actually calls #array_parents).
This is used in the AJAX 'add more' handler, and in the assignment of field validation errors down to actual form elements.
This information bloats $form and makes a print_r($form) tedious. This belongs in $form_state, but couldn't live there before $form_state persistence and auto-rebuild behavior got cleaned.
Also, not all FAPI callbacks receive $form (some receive an $element), but all receive $form_state.
1. removes $form['#fields'] and introduces $form_state['storage']['field_info'] - for each field:
$form_state['storage']['field_info'][$field_name][$language] = array('field' => $field definition,
'instance' => $instance definition,
'array_parents' => (previously 'form_path') FAPI #array_parents : path to the field elements in $form
'errors' => the array of field-level errors reported by hook_field_validate()
);
2. fixes widgets that were incorrectly relying on field_info_[field|instance]() instead of $form['#fields'], and also clearly documents that practice in hook_field_widget() (actually that's still a @todo in this version of the patch)
3. makes field_add_more_js() and field_default_form_errors() use the $form_state 'array_parents' info (prevsiously used the info from $form['#fields']).
4. automatically populates 'array_parents' at #after_build time.
5. adds $form and $form_state args to hook_field_widget_error(). That's a widget hook, those arguments belong here, even if most implementations probably won't need them (current ones in core do not). API addition rather that API change, since nothing breaks if a given hook overlooks those additional args.
'crtical' because with the current code, a contrib module form_altering the structure of the $form without updating $form['#fields']'s 'form_path' accordingly causes field-level validation errors not to be reported on the form and thus bypassed.
#16
Overall, I like all of the changes in this patch. I was worried about #after_build before, but then again, that's probably the right point to do this, and after all, the originally intended purpose of #after_build.
+++ modules/field/field.form.inc 4 Jan 2010 19:41:48 -0000@@ -17,16 +17,16 @@ function field_default_form($obj_type, $
+ // Store field information in $form_state['storage'].
+ $form_state['storage']['field_info'][$field_name][$langcode] = array(
Since we removed the limitation of $form_state['storage'], we should now store this information by module name, i.e.
$form_state['field']
Powered by Dreditor.
#17
So let's see what the bot thinks of this (in-patch-rename).
#18
Also, tagging.
#19
I agree with the change.
#20
All in all, it's probably still sun who can RTBC ;-).
#21
We still need to fill out those @todos though.
#22
Ah true, I left dome docs @todos pending for an initial approval of the approach :-/
Will try to add those asap.
#23
Fixed the doc @todos.
Patch summary is in #15.
#24
Awesome!
+++ modules/field_ui/field_ui.admin.inc 15 Jan 2010 20:31:48 -0000@@ -1196,6 +1196,7 @@
+ '#parents' => array(),
uhm, err, why that? :)
+++ modules/field/field.attach.inc 15 Jan 2010 20:31:41 -0000@@ -400,6 +400,79 @@
+ * '#field_name' => the name of the field
Missing trailing commma.
+++ modules/field/field.attach.inc 15 Jan 2010 20:31:41 -0000@@ -400,6 +400,79 @@
+ * // Only for 'single' widgets:
+ * '#theme' => 'field_multiple_value_form',
Wrong indentation for inline comment.
+++ modules/field/field.attach.inc 15 Jan 2010 20:31:41 -0000@@ -400,6 +400,79 @@
+ * 'field_foo' => array (
+ * $langcode => array (
Spaces before opening parenthesis.
+++ modules/field/field.attach.inc 15 Jan 2010 20:31:41 -0000
@@ -414,73 +487,8 @@
+ * parameter. Processing information is added by reference in
+ * $form_state['field'].
+++ modules/field/field.form.inc 15 Jan 2010 20:31:41 -0000
@@ -17,16 +17,16 @@
+ $form_state['field'][$field_name][$langcode] = array(
'field' => $field,
'instance' => $instance,
+ // This entry will be populated at form build time.
+ 'array_parents' => array(),
+ // This entry will be populated at form validation time.
+ 'errors' => array(),
);
The docs state that the values are added by reference, but this doesn't seem to be the case?
+++ modules/field/field.api.php 15 Jan 2010 20:31:39 -0000@@ -609,15 +609,25 @@
+ * (regular object edit form, 'default vale' input in the field settings form,
Typo in 'vale' here.
+++ modules/field/field.api.php 15 Jan 2010 20:31:39 -0000@@ -609,15 +609,25 @@
+ * - $form_state['field'][$field_name][$langcode]['field'],
+ * - $form_state['field'][$field_name][$langcode]['instance'].
We can remove the trailing punctuation here.
Powered by Dreditor.
#25
+++ modules/field_ui/field_ui.admin.inc 15 Jan 2010 20:31:48 -0000@@ -1196,6 +1196,7 @@
+ '#parents' => array(),
That's for the widget element used as 'default value' input in the field settings form. That change goes along with those in the _validate and _submit form handlers:
+++ modules/field_ui/field_ui.admin.inc 15 Jan 2010 20:31:48 -0000@@ -1217,25 +1214,22 @@
- // Extract field values.
- $items = array();
- $form_state_copy = array('values' => $instance['default_value_widget']);
- field_default_extract_form_values(NULL, NULL, $field, $instance, LANGUAGE_NONE, $items, $form, $form_state_copy);
(...)
+ $items = array();
+ field_default_extract_form_values(NULL, NULL, $field, $instance, LANGUAGE_NONE, $items, $form, $form_state);
It's a fine change, except I guess it doesn't really belong in this patch now (the initial patch in the thread ventured into 'parents' and $form_state ['values'] land).
I removed those hunks from the patch.
Hm, the 'array_parents' and 'errors' entries are initialized as empty arrays by ref in f_attach_form() / f_default_form(). The doc also says that they are actually populated respectively at form build time and form validation time.
Other remarks fixed.
Powered by Dreditor.
#26
Spin off for the
'#parents' => array()hunks in field_ui 'edit default value' : #686226: Nicer code in 'default value' input handling#27
Somehow I only thought I'd have this marked as RTBC already. ;) Good job, yched!
#28
Yikes. This is a huge patch, but seems to mostly just be making the same basic changes over and over.
I was going to commit this, but it no longer applies. Can I have a re-roll please?
#29
Rerolled - was a hunk for a trailing whitespace fix that got committed meanwhile.
#30
Thanks! Committed to HEAD.
#32
#7: field_form_state-641670-7.patch queued for re-testing.
#33
Not sure why this was reopened ?
#34
Automatically closed -- issue fixed for 2 weeks with no activity.