Convert field form data to object properties

We encourage users to post events happening in the community to the community events group on https://www.drupal.org.
bjaspan's picture

I think we need another Field Attach API function.

Currently, a fieldable entity calls field_attach_form($obj_type, $obj, $form, $form_state) to load the appropriate field elements into $form for editing $obj. Later, it calls field_attach_validate(..., $form, $form_state) so the field widgets can perform their validation. Eventually, it will call field_attach_insert($obj_type, $obj) or field_attach_update($obj_type, $obj) to store the field data in the database.

Question: How does the entity implementation convert the field data in $form_state into object properties on $obj (e.g. $form_state['field_myimage'] to $node->field_myimage)?

The current answer, used throughout Drupal core and contrib as well as Field API, is to cast the $form_state array into an object, pre-supposing that you built the form in such a way that it maps directly to the object structure. This is in fact how node and user currently do it. However, it is a horrible approach. It constrains form designs to map to object structure, when some radically different form might provide better UX. It also leads to developer confusion about the "real" representation of certain data structures, which is for example why we have code in which $node is an object, an array, or either!

I believe as a matter of principal that form submission data and objects built from it should be disconnected; the latter should be built up explicitly from the former, piece by piece, so that the object structure is known to be valid when it is then passed to some object API (e.g. node_save()). This should be the role of hook_nodeapi_submit: each module adds properties to the $node object based on what it finds in $form_state from the form elements that module added.

I can think of three approaches for Field API:

  1. Adding #validate and #submit handlers to the field form elements added by field_attach_form (I think Karoly argued for this during the sprint). The #submit handler would then save the data directly based on $form_state. I think this is the wrong idea. We would have to build in too much assumption about what should happen to the data on form submission. We'd have to know which buttons the form contains (Preview, Save, Delete, etc.) and what they are supposed to do. etc.

  2. field_attach_form_submit($obj_type, &$obj, $form, $form_state). The fieldable entity would call this from its own submit handler. $obj would be modified to contain the correct field properties based on what is found in $form_state.

  3. field_attach_form_extract($obj_type, $obj, $form, $form_state). The fieldable entity would call this from its own submit handler. The function would return an array of field properties (e.g. array('myimage' => array([0] => array('fid' => 123, ...)))) which the fieldable entity would then merge into $obj before saving.

I think #2 makes the most sense given the rest of our API. I know that hook_nodeapi_submit() has a made reputation because any CCK field or module that uses is ends up forcing the use of drupal_execute(). However, that is only because CCK and those modules did not/do not have decent APIs that are separate from the Form API. Field API does have a decent API, and field_attach_form_*() are the Field API functions that are designed to work with Form API form submissions, so there is no problem with using them that way.

Incidentally, I also think we should rename field_attach_validate() to be field_attach_form_validate().

comments?

Comments

Later, it calls

yched's picture

Later, it calls field_attach_validate(..., $form, $form_state) so the field widgets can perform their validation
Not the point here, but this isn't accurate. Widget-level validation is done through FAPI's #element_validate, and is thus already done when we get to field_attach_validate(), which lets fields do their own validation through hook_field_validate(), for cross-widget stuff like 'is the number between min and max', that widgets should not have to care about.

Other than that, +1 on the principle of field_attach_form_submit, I think (it's late :-).

I also vote for 2). When other field_attach_*() functions operate on the fields entries in the $object, they do so by reference :
field_attach_load(), _validate(), _presave(), _prepare_translation()...
Conceptually, field_attach_form_submit() is similar to field_attach_load(), except we populate values from incoming form data rather than the db. l like that :-)

The point is we have already

KarenS's picture

The point is we have already tried to separate widget validation from field validation. Widget validation isn't needed unless you are using that widget for input and is specific to the widget being used, including massaging the data from whatever form it has in the widget to the form the field expects. Then field validation picks up to ensure that the data is valid data for the field before it is accepted into the db.

Field validation should happen always, including data created from the API. Widget validation is meaningless unless values are input using the widget. So widget validation happens in FAPI but I don't think field validation should have anything to do with FAPI since it should happen no matter where the data is coming from.

Just wanted to clarify that, I'm still mulling over the rest of this post :)

Field validation should

yched's picture

Field validation should happen always, including data created from the API

That's right, of course. But there are a few challenges :

  • AFAIK, non-FAPI validation doesn't exist anywhere in core, specifically not on 'fieldable' candidates (node_save, user_save, taxonomy_term_save...), so we'd have to settle sound and consistent conventions for 'exception handling / error codes on *_save()'. This has a fairly large 'philosophical' scope IMO and will potentially raise lots of debating.

  • Form vs. non-form workflow : node_save can't perform validation itself, since it's called at 'FAPI submit' time, so it's too late to raise form validation errors there. field_attach_validate() still needs to be called at form validation time.
    So we'd need a separate node_validate_and_save() function for programmatic node save, that does its own call to field_attach_validate(), and then to node_save() (we might want to rename node_save() to _node_write() and node_validate_and_save() to node_save(), if we want to keep clean API names).

  • hook_field_validate() needs to be able to report errors when in 'form submit' context (form_set_error() ) and when in 'programmatic' context (raise exception), so we'd probably need to find a way to make the job easier on field-type modules (read 'we should not trust field modules with that'). Possibly : let hook_field_validate() return an array of errors, and have field_attach_validate() handle the actual error reporting.
    We'd probably need to find a cleaner way to determine the exact form element that should be flagged with the error. The current $item['_error_element'] thing is a sore hack.
    [edit - the last remark probably needs a little explanation: the issue is that the field cannot make any assumptions on the form structure of the widget. Currently, widgets artificially stick an '_error_field' entry in $item values that lets hook_field_validate know where to flag errors. Yuck.]

Karen, thanks for the

bjaspan's picture

Karen, thanks for the clarification. I knew as I was writing this post that the widget should be responsible for massaging the post data into the correct field structure, and I had a nagging feeling you and Yves had already worked some/all of this out, but of course I did not take the time check to the code before spouting off. :-)

Yves, however, points out a whole set of excellent reasons that we have a bit of a problem if we want field validation to occur outside of FAPI. The bottom line is that Drupal does not work that way; node_save() always just saves what you give it, even if it is "invalid." Drupal APIs should have non-form-based validation, but they do not. This is just another core Drupal architectural problem that we should not try to fix via the Field API patch.

I am thus tempted to stick with the (broken) approach we have now: Widgets perform their validation via FAPI. Fields perform their validation via field_attach_validate() (which I guess should not be renamed to field_attach_validate_form()), but field_attach_validate() has to assume it is being called from inside a form submit handler (which is currently assumes when it calls form_set_error()). The fact that CCK mostly separates these two kinds of validation will make it easier to move to a less-broken API in the future.

Actually, here's an idea based on what Yves said for improving the logical separation of widget vs. field separation in the current code:

  1. We change hook_field_validate() so instead of being allowed to call form_set_error(), it returns a list of errors, keyed by delta. Easy change.
  2. field_attach_validate() uses _field_invoke() as it does now to call all the field validators. _field_invoke() returns an array of all the hook_field_validate() return values, but keyed by field name instead of just merged into an array. field_attach_validate() then just returns that results array, or maybe throws a FieldValidationException which contains the array (not just a serialized string). This also sounds easy.
  3. We create field_attach_validate_form(), which is what fieldable entities should call from their form validation handlers. It calls field_attach_validate(), and gets back a list of errors (either by return value or by catching the exception), keyed by field. For each field with errors, it looks up that field's widget and passes it the errors. again, easy.
  4. The widget knows how to call form_set_error() based on which delta got an error because the widget is what generates the form. I guess this might be a bit of work, but perhaps that is only because I do not understand how #error_element currently works.

This gives us separation of widget validation and field validation in a way that preserves the ability to set form errors based on field validation errors. Also, "some day," when node_save() is allowed to fail, field_attach_insert()/field_attach_update() can then call field_attach_validate() to check for errors and throw a FieldValidationException if any errors are found.

I do not think this is necessary for our initial commit; we can continue being just as broken as the rest of core.

Comments?

Sounds like an excellent

yched's picture

Sounds like an excellent plan, I do think we should go for it (although, agreed, not critical for the initial commit).

Gets more complicated around 3 and 4 and 'error element', though :

3. (...) For each field with errors, field_attach_validate_form() looks up that field's widget and passes it the errors

Through a new hook_widget_flag_error() for widget modules, right ?

4. The widget knows how to call form_set_error() because the widget is what generates the form.

Unfortunately this is a little more complicated :
a) The widget knows its own structure inside the original $form['field_foo'] element, but that element can later be form_altered to another spot in the $form (think fieldgroup.module and $form['fieldgroup_bar']['field_foo']). It's only at process-time that the widget knows its #parents and can determine the full path to the 'potential error element' in the $form - see the bottom of text_textfield_process().
b) this 'path to potential error element' has to make its way from text_textfield_process() up to (currently) field_text_validate() where we need to know it. How do we do that ?
- text_textfield_process() can write to $form_state but in D6 hook_nodeapi('validate') did not let us carry $form_state up to field_text_validate()
- field_text_validate() can receive $form, but reference-writing into it in text_textfield_process() doesn't 'take'.
So we resorted to putting it in a fake form value that ends up in the submitted $items

But the good news is that in your approach, field_attach_validate_form() does receive $form_state, and can propagate it to hook_widget_flag_error(), which is were we now need the 'path to error element'.

So widgets
a) write the error element into $form_state during their hook_process(),
b) read it form $form_state back in hook_widget_flag_error().
Maybe parts of this could be 'automated' by field_attach's built-in form handling, instead of currently entirely on the shoulders of widgets.

Note : a similar mechanism will probably be needed to make the JS-'add more' button work when form parts get moved around in form_alter - see my 'TODO' comment for D7's field_add_more_js().

Yes, a new

bjaspan's picture

Yes, a new hook_widget_flag_error() or something like that.

Quite frankly I am terrified by field FAPI code. I'm hoping you or Karen will do this. :-)

I added it to the planning spreadsheet, unprioritized, story name "Improve field vs. widget validation."

Right. The rest of the APIs

yched's picture

Right. The rest of the APIs and processing gained a lot from the many eyes and team work. Form handling code is the last dark place.
My secret hope is that, once it goes in core, FAPI gurus will be forced to look at it and give it some tough love and brilliant ideas :->

slight gotcha : complex

yched's picture

slight gotcha : complex widgets can be composed of several form elements, and accurately pointing the user to the exact invalid input means different errors might need to be flagged against different elements in the widget. So the errors returned by hook_field_validate() would need to have an error code (NUMBER_ERROR_GREATER_THAN_MAX), that the widget can use to direct the right error to the right form element.

Those error codes would probably also be useful when we raise exceptions, later on...

What is an example of a

bjaspan's picture

What is an example of a current complex widget/field setting combo that would cause this that we currently handle in some way that is better than what's possible with what we're discussing here?

Long answer would be painful

yched's picture

Long answer would be painful to write and read, but short answer is: no, AFAICT we don't lose flexibility or make things (even) more convoluted. The 'new' way we're discussing is a net gain:
- field hooks do not need to care about widgets, they just report errors
- widgets are free to handle the errors according to their own internal structure
My last remark was only me realizing that some complex widgets will need to be able to recognize different sorts of field validation errors (hence error codes, not just human text), and flag them to the right sub-element (no actual example off the top of my head - maybe around date, filefield, asset...)

I'll try to wrap some code when I get a chance, unfortunately I'm in a over-busy month :-)

yes please.

catch's picture

We did this for taxonomy module - _load _save _delete and their respective hooks all operate on term objects - a lot to clear up there still but it's on it's way. It'd be great to have fields set an example for this kind of consistency (and try to get node and user done before D7 is released as well).

Fields in Core

Group organizers

Group notifications

This group offers an RSS feed. Or subscribe to these personalized, sitewide feeds: