Currently value assignment for elements in core are hard coded in form_builder as a switch statement. This patch allows form elements to specify a _value hook for assigning both #value and #default_value.

This patch also creates a rough equivalent of the nodeapi 'prepare form values' phase.

doc: The modified _value callback operates in two modes. value mode and default value mode. if you pass two arguments (form element & matching edit array) it should return the value for the form element. If you only pass in a form element or form element and FALSE for the second argument it should return the default value for the form element.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dopry’s picture

I should note that the patch was originally written by chx, I just did bug fixing, testing, and documentation.

chx’s picture

Status: Active » Needs review

note that as $edit is always a string or NULL it can't be FALSE. Also, the point of this patch is to enable more power with custom types.

yched’s picture

Maybe it's too early for that, but wouldn't it be useful to allow a custom callback (as is the case for 'validate'), and if none use the hardcoded '#type'_value pattern

Could allow different widget types to share _value callbacks, or, on the opposite, use the same widget to edit different data types (text goes right through, uid is reformatted as username in the input field)

dopry’s picture

@yched,
I hadn't thought about that, but I don't think that overriding value assignment from _POST for elements would be a wise idea. Many things are dependent on #value. I think cases where people need to override it would probably be good cases for new custom elements. Do you have any use cases in mind?

chx’s picture

Changing user input simply will not go through, Drupal does not change user data on input.

yched’s picture

User input is not what I'm after, but rather the #default_value the form element gets pre-filled with.

I guess I'm reacting to the rough equivalent of the nodeapi 'prepare form values' part in dopry's original post.

My suggestion aims at providing a way to have the same widget be used for editing data that won't require the same 'prepare form value' massaging steps.
Say, a 'textfield' widget can be associated to a 'text' field - the #default_value is the text value 'straight out of the db'.
But it can also be associated to an "author" field - the field stores a uid, which has to be translated to provide the user name as the #default_value.

This comes from the recent 'CCK in core / FAPI3' discussion in devel ML, and my playing with adrian's initial code. But the status of FAPI3 is yet pretty much uncertain, so these considerations are probably (at best) premature and just adding noise around the patch. Sorry about that.

fago’s picture

this patch makes contributing form elements easier, so I think such functionality is much needed. It's still a bit different from the nodeapi 'prepare' phase, as it can be only used by the implementing element type, but imho that's what is needed.

yched: I think this would make sense as soon as there is a widget - data separation in the form API itself.

dopry’s picture

Yes, but did you test the patch? Do you have questions you wish to address to me regarding the patch? Is the code style ok? Does it make baby jesus cry or gurgle?

fago’s picture

ah sry, of course I've tested it. It worked without any troubles - I've tested several form (-types). Code style is great, in addition the code is structured a lot better than before.
Conclusion: A much needed, well-written, clean patch: +1 for it!

yched’s picture

Status: Needs review » Needs work

password_confirm widget seems broken.
the pass1 and pass2 sub-elements never get a #value, and thus get submitted with the #default_value (empty string)

As a side note : I find the mixed style (return value + $form altered by reference) for the _value callbacks produces code that's not easy to follow. But maybe it's just me :-)

fago’s picture

Status: Needs work » Needs review
FileSize
5.81 KB

ah, yes. the problem is, that the current code doesn't detect if the function does handle only the default_value case or both.
I've attached an updated patch, which fixes this.

+        if ((isset($form['#value']) && $form['#value'] === $edit) || (isset($form['#required']) && $form['#required'])) {
+          $form['#needs_validation'] = TRUE;
+        }

Furthermore $form['#value'] is now not necessary more equal to $edit, so I removed the check for being equal.

Then I have to agree with yched. But altering $form shouldn't be necessary, as it its value won't be used, if the function does already handle setting the default_value. So I just changed $form to be passed as paramter, so that this isn't so confusing any more.

Please review.

chx’s picture

Minor detail: we do not use is_null. isset is the same just faster.

chx’s picture

Otherwise patch reads good. Reviewers: please check that you are possible to uncheck a single checkbox and also remove all from a checkboxes element and that user password edit works.

fago’s picture

FileSize
5.81 KB

ok, attached is a patch without is_null()

yched’s picture

Status: Needs review » Reviewed & tested by the community

per chx's review list :
uncheck a single checkbox : OK
remove all from a checkboxes element : OK
user password edit works : OK
single / multiple select also seem OK

Since chx already +1'd the coding style, I'm setting to RTBC ?

Dries’s picture

Just postfixing the type with '_value' seems a little tricky. I'd prefer to use form__value so they are in the form_ namespace, or to have some sort of callback mechanism. Also, 'value' is quite a generic name. Not sure but do we have ideas for something more descriptive?

This patch makes me wonder how we could do a better job supporting custom form API types.

dopry’s picture

FileSize
5.88 KB

This patch changes the namespace for the value callback to 'form_type_'. $element['#type'] .'_value'.

Dries’s picture

dopry: thanks -- looks better. I'd still love to get some feedback from the others. Is this the right approach to make widgets?

chx’s picture

What to say? The implementation is based on my code so I can't say I dislike the approach. The most important difference is that it actually works :D -- thanks guys for running with the patch. yched went over the stuff that just always breaks when touching fapi innards so as fare as I am concerned -- let's go with it.

dopry’s picture

Title: FormAPI #type.'_value' callback, remove hardcoded switch from form.inc. » FormAPI #type .'_value' callback, remove hardcoded switch from form.inc.

bump. Is there more that needs to be done, further changes or clarification that is necessary? This patch makes for elements atomic and allows modules to implement more inteliigent form_elements.

dopry’s picture

Category: feature » bug

I'm switching this to a bug. It is an implementation flaw in form api that allows modules to define all the behavior for form_elements except value assignment. I'd really like to see this land or someone to improve and add a #value_callback or similar. This patch is a big enabler for exploring widget like form elements, and creating more intelligent form elements.

This patch isn't really about widgets, while it could be a basis for widget work, It about removing hard coded value assignment from form_builder, and allowing developers to implement new form elements without patching form_builder.

*This ends yet another comment to get the issue back on the radar.*

drewish’s picture

subscribing...

eaton’s picture

Taking a look at this, I agree wholeheartedly. The FormAPI 3 refactoring chx and I put together tries to split out the handling of value assignment a bit already -- making it explicit, the way this patch does, is a good thing. +1 from me.

drewish’s picture

Status: Reviewed & tested by the community » Needs work

patch doesn't apply:
Hunk #1 FAILED at 691.
Hunk #2 succeeded at 813 (offset 3 lines).
Hunk #3 succeeded at 1284 (offset 3 lines).

drewish’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

here's a re-roll.

drewish’s picture

FileSize
5.97 KB

here's a re-roll post fapi3 patch.

drewish’s picture

this still applies with a little fuzz.

eaton’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.98 KB

+10 or so. Again, this is a very valuable addition. The only thought I have is that it might be worth defining a #value_assignment_function, or something like that, in the element definition rather than always building based on the #type. That's a potential for future improvement, and shouldn't prevent this patch from going in.

Rerolled with no changes against the latest head -- should apply without any offsets.

Dries’s picture

This looks ready. However, could we add some PHPdoc to the new functions. It should be straightforward to add this, but it helps people understand what these are for, where they are called from, and what the $edit parameter is up to. +1 on the functionality, -1 on the documentation. ;-)

drewish’s picture

FileSize
7.39 KB

i've replicated the bit of phpdoc that eaton had in his patch to each of the functions so that something descriptive will show up on api.drupal.org. if something more substantial is required i'm not sure i'm the best person to do so (i've just been pushing this patch because dopry mentioned it'd make it easier to add a file value handler).

eaton’s picture

*bump*

This is a pretty important patch, and helps pave the way for serious improvements to CCK during the D6 lifecycle. We may not be getting many UI components from CCK into core this time around, but underlying stuff like this will help us solve several of the module's pressing problems cleanly.

yched’s picture

Just chiming in. This is indeed a needed step for one of the main aspects of the (now mythical...) CCK 2.0, be it core or contrib : move widgets closer to FAPI - and be able to internalize more widget and multiple value handling in content.module

Patch is still RTBC, and the code comments Dries asked for have been added, so let's get this in ?

Dries’s picture

I'd like to see us work on the PHPdoc some more.

"Relevant post data for real value, or FALSE for default value"
"Mixed, value to be assigned to element."

The return value is often NULL (i.e. when $edit is FALSE). This is not reflected in the PHPdoc. When can NULL be returned and when not?

I think it would be good to point out that the default values are in the form description.

The other patch in the patch queue seems to merge $form_states and $form_values? Can we expect to see this happen here too?

We seem to mix !empty($edit) and isset($edit).

I think this patch could use a tad more polish.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

It should be easy to get this patch in, but let's take time to do it right.

eaton’s picture

regarding the $form_state vs $form_values switch, if this one goes in first I'll change the $form_state patch, and if that one goes in first I'll change this patch. ;) It's just tricky to keep both of them in sync.

The other changes, yes, I'd agree.

dopry’s picture

Status: Needs work » Needs review
FileSize
7.09 KB

@Dries, can you elaborate on the isset vs !empty comment. They aren't equivalent tests. However, I did find some minor improvement using NULL as the default value for $edit in the _value callback function prototypes.

eaton’s picture

Status: Needs review » Needs work

The change from FALSE to NULL for defaults seems to have broken checkboxes; I'm investigating.

eaton’s picture

Status: Needs work » Needs review
FileSize
8.12 KB

Re-rolled it, with some slightly cleaned up PHPDoc comments and a fallback use of FALSE instead of NULL. checkboxes work again. :)

eaton’s picture

Also, added the ability to set #value_callback on form elements so that CCK widgets, etc can share their value-processing code.

eaton’s picture

To bump and summarize, we need this patch because:

1) It simplifies the monster, monolithic form_builder function further and makes the code more granular.
2) It makes implementing new element types that do funky things with their values a lot simpler.
3) It makes FAPI flexible enough to do most of what CCK does with its 'editor widgets'. That means that one more backend element of CCK has, essentially, made it to core.

pwolanin’s picture

patch code looks reasonable, but I haven't tested it yet.

asimmonds’s picture

I've gone through quite a number of core forms, testing if their functionality hasn't been broken by this patch.
Also tested a custom form with all the different form element types (needed a patch or two from some other open issues for everything to work properly)
So far, haven't found anything that has been broken by this.

KarenS’s picture

I tried this with a lot of the core forms and everything seemed to work as it should.

KarenS’s picture

Status: Needs review » Reviewed & tested by the community

I have now also tested this by creating a custom form element and adding it to a core form with form_alter(), then tried giving it various default values, or no default value. It all seems to work just fine.

Big +1 from me on this.

I'm going to venture to mark this RTBC. It appears to break nothing in the current form handling but gives us some ways to get some CCK-type functionality into FAPI. As Eaton says, this will be another step closer to CCK in core.

yched’s picture

... and it will also let contrib CCK for D6 simplify the widgets API (even if that might not be considered a decisive argument for the core inclusion of this patch)

Big +1 form me as well :-)

pwolanin’s picture

If I have time, I'll go through the code in more detail, but it works fine at a functional level, and creating callbacks works to submit a derived or arbitrary value.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

eaton’s picture

And the ewoks danced!

Thank you, Dries. This small patch is the cap on a lot of hard work by chx, Dopry, and myself in making FAPI3 the foundation for a leaner, meaner, easier-to-grok CCK 2.0. Hooray!

dopry’s picture

/me wipes the sweat from eaton's brow and offers him a drink.

yched’s picture

Not sure if that's intentional, but it seems the patch that got committed is not the right one ?

Eaton's last patch in #38 had that (powerful) line :

$function = !empty($form['#value_callback']) ? $form['#value_callback'] : 'form_type_'. $form['#type'] .'_value';

where current 6 has :

$function = 'form_type_'. $form['#type'] . '_value';

The #value_callback stuff was a precious enabler for CCK's widget refactoring.

eaton’s picture

Status: Fixed » Needs work

Auuuugh, I didn't realize that the *old* version of the patch got applied. Dries, Goba, was this intentional? Hopefully not :-/

KarenS’s picture

I also would like to see Eaton's version get in if at all possible. That's the one I tested in #43 and #44, and that's the one that would be most useful to help us get more FAPI into CCK.

AjK’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
6.88 KB

see also duplicate issue: http://drupal.org/node/158665

The wrong patch got commited on this issue leaving single item checkboxes broken :( But a lot of water passed under this bridge since it was committed as it's gone unnoticed.

Eaton's correct patch was rolled against 1.207

This patch goes back to 1.207, applies the correct patch, and then moves forward re-applying patches upto the latest to bring this back to 1.215 (skipping the duff commit obviously).

asimmonds’s picture

I don't think this line should be removed:

@@ -708,7 +708,6 @@ function form_builder($form_id, $form, &
   if (isset($form['#input']) && $form['#input']) {
     _form_builder_handle_input_element($form_id, $form, $form_state);
   }
-  $form['#defaults_loaded'] = TRUE;
 
   // We start off assuming all form elements are in the correct order.
   $form['#sorted'] = TRUE;

It's from the #pre_render callback patch #147662,
rev 1.208
of form.inc.

eaton’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6.61 KB

Here's the corrected patch, changign only the lines that were left out of the committed version of the _value patch.

marking RTBC as it only brings things into sync with the previously tested and verified RTBC code.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

eaton’s picture

Awesome, Dries. Thanks!

Anonymous’s picture

Status: Fixed » Closed (fixed)