| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | critical |
| Assigned: | yched |
| Status: | closed (fixed) |
| Issue tags: | D7 API clean-up, Fields in Core |
Issue Summary
As explained in #414424-43: Introduce Form API #type 'text_format' and below, the current pattern used by core widgets of defining a specific FAPI #type expanded into 'common' FAPI #types (textfield, textarea, etc...) is a CCK D6 attempt at reusability that didn't fully live up to its expectations, is not in line with the directions of D7, and makes a code workflow that is both convoluted and extremely painful to interact with in form_alter().
While this approach might still make sense for some specific widgets on some specific field types and remains possible, widgets shipped in core should be done 'the simple way' (which was in fact not possible before #567064: Widgets done "the easy way" have too many limitations), to serve as example for contrib field types..
#414424: Introduce Form API #type 'text_format' is already doing this for text widgets, here's a patch for number.
Other widgets will follow.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| field_number_simplify.patch | 12.84 KB | Idle | Passed: 14694 passes, 0 fails, 0 exceptions | View details |
Comments
#1
Awesome! Much nicer code!
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+ // Substitue the decimal separator.
(minor) Typo in Substitute.
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
- $element[$field_key] = array(
+ $element += array(
...
- $element[$field_key]['#field_prefix'] = field_filter_xss(array_pop($prefixes));
+ $element['value']['#field_prefix'] = field_filter_xss(array_pop($prefixes));
...
- $element[$field_key]['#field_suffix'] = field_filter_xss(array_pop($suffixes));
+ $element['value']['#field_suffix'] = field_filter_xss(array_pop($suffixes));
The 'value' key seems to be wrong.
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+ // A specific CSS class is needed to override node from default styling and
s/from/form/ ?
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+ // Prefix and suffix.
Prefix with "Handle "?
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
- return $element;
+ return array('value' => $element);
Hmmm... Either we should build the element in 'value' upfront, or we should simply set #parents accordingly. I'd highly prefer the latter. However, it's possible that we do not setup #parents in the passed $element. So that would be something we should consider.
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+ $type = str_replace('number_', '', $field['type']);
strtr() is faster.
We perhaps should also add an inline comment that clarifies the presumption that the 'type' without number_ prefix is supposed to be the name of a number type.
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+ break;
+ case 'integer';
There should be a newline in between separate cases that do not belong together or include a fall-through logic.
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+ // @todo : this should be done at field level.
No colon after @todo and start with an uppercase letter to form a proper sentence.
+++ modules/field/modules/number/number.module 7 Nov 2009 18:21:46 -0000@@ -296,185 +282,88 @@
+}
\ No newline at end of file
This should not happen.
I'm on crack. Are you, too?
#2
Tagging.
#3
Much, much better. Hurray for simpler widgets
#4
Updated for sun's comments.
We now provide #parents in the base $element received by hook_field_widget(). This additionally makes sure that field values end up where we expect them in field_default_extract_form_values(), even if the form structure is altered for presentational purposes.
#5
The last submitted patch failed testing.
#6
I removed the #parents stuff, it has consequences that are not fully clear to me yet, required me to patch filefield as well, and I don't want to derail the overall simplification done here.
I don't think we got #567064: Widgets done "the easy way" have too many limitations fully right (see my remark in #414424-60: Introduce Form API #type 'text_format'), and the question of whether we can pass #parents in a way that makes sense can be sorted out later.
As sun pointed on IRC, field_default_form() does need to set some #parents on the wrapping containers to ensure that field_default_extract_form_values() finds submitted field values where it expects them, whatever form_alters might have been performed on the form structure, but that's for a different issue.
So, this patch fixes sun's comments in #1, except:
So my answer is:
- #parents would be something to consider, but in a followup ;-)
- I can't build the element in 'value' upfront because of the $element arg received by hook_field_widget().
If the arg was named $base or whatever, I could do
$element['value'] = $base + array('#type' => 'textfield',
...
);
but I can't do
$element['value'] = $element + array('#type' => 'textfield',
...
);
#7
ok, agreed, separate issue. I guess this will pass again.
#8
Thanks sun ;-)
Spin-offs:
#627730: Make field forms resilient to form_alter() structure changes
#627204: Unnecessary prefix on field 'add more' $form element
#9
mmmmh... I reviewed this once again, so I actually meant RTBC ;)
#10
Yes, sorry, FF reload playing tricks on me.
#11
Nice clean up. Two minor suggestions:
+++ modules/field/modules/number/number.module 9 Nov 2009 15:07:43 -0000@@ -39,7 +30,7 @@
- 'settings' => array('precision' => 10, 'scale' => 2, 'decimal' => '.'),
+ 'settings' => array('precision' => 10, 'scale' => 2, 'decimal_separator' => '.'),
The naming seems inconsistent here. Should be 'separator' or 'decimal_precision' and 'decimal_scale'. Either prefix with 'decimal_' or don't at all. I'd prefer just 'separator' because it is also used for floats.
+++ modules/field/modules/number/number.module 9 Nov 2009 15:07:43 -0000@@ -296,185 +282,90 @@
+ // A specific CSS class is needed to override node form default styling and
Should be "the node form's" instead of "node form", not? Slightly better English.
"and allow" should be "and to allow", I think.
Should be a quick reroll! Great work.
#12
Not really, actually. 'decimal' in 'decimal_separator' is not a prefix, it's the full name of the property: 'the decimal separator'. As such, it is also a valid name for floats.
On the display side, formatters already have 'thousand_separator' and 'decimal_separator'. We just make the name of the setting consistent between the widget side and the formatter side. Widget's don't have a 'thousand separator' because it's more a display thing than an input thing, but I'd rather have the setting formatters and widgets do have in common to have the same name.
I'd also rather avoid 'decimal_precision' and 'decimal_separator', because 'precision' and 'scale' map directly to SQL and schema API properties for decimal columns, and also because, precisely, we don't prefix the name of our settings ;-).
So I'd advocate for the names in the patch. Rerolled patch fixes the 'node form' sentence.
Bouncing back to RTBC for Dries.
PS: meanwhile, and after an nth plea, I finally learned to roll a diff -up patch on Win / Eclipse...
#13
"Bouncing back to RTBC for Dries", he said.
#14
Next in line: #628188: Remove #process pattern from taxo autocomplete widget
#15
@yched, fair enough. Thanks for the clarification. Committed!
#16
Followup for the @todo added in this patch: #631048: Number rounding should not be done by widgets
#17
Next in line: #635202: Remove #process pattern from option widgets
#18
Automatically closed -- issue fixed for 2 weeks with no activity.