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.

CommentFileSizeAuthor
#12 field_number_simplify.patch13.19 KByched
#6 field_number_simplify.patch12.89 KByched
#4 field_number_simplify.patch16.49 KByched
field_number_simplify.patch12.84 KByched

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Move core widgets out of the #process pattern » Remove #process pattern from number field
Priority: Normal » Critical
Issue tags: +Fields in Core, +D7 API clean-up

Tagging.

moshe weitzman’s picture

moshe weitzman commented

Much, much better. Hurray for simpler widgets

yched’s picture

yched commented
StatusFileSize
new field_number_simplify.patch16.49 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new field_number_simplify.patch12.89 KB

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:

+++ 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.

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',
  ...
);
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

ok, agreed, separate issue. I guess this will pass again.

yched’s picture

yched commented
Status: Reviewed & tested by the community » Needs review

Thanks sun ;-)

Spin-offs:
#627730: Make field forms resilient to form_alter() structure changes
#627204: Unnecessary prefix on field 'add more' $form element

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

mmmmh... I reviewed this once again, so I actually meant RTBC ;)

yched’s picture

yched commented

Yes, sorry, FF reload playing tricks on me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

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.

yched’s picture

yched commented
StatusFileSize
new field_number_simplify.patch13.19 KB

The naming seems inconsistent here.

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...

yched’s picture

yched commented
Status: Needs work » Reviewed & tested by the community

"Bouncing back to RTBC for Dries", he said.

yched’s picture

yched commented

Next in line: #628188: Remove #process pattern from taxo autocomplete widget

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

@yched, fair enough. Thanks for the clarification. Committed!

yched’s picture

yched commented

Followup for the @todo added in this patch: #631048: Number rounding should not be done by widgets

yched’s picture

yched commented

Next in line: #635202: Remove #process pattern from option widgets

Status: Fixed » Closed (fixed)
Issue tags: -Fields in Core, -D7 API clean-up

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Fields in Core, +

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.