Refactor field validation and error reporting

So, a little background information :

CCK concepts reminder:
There are two levels of validation for fields in forms: widget-level validation, and field-level validation.
- Widget validation steps are specific to a given widget's own form structure and input format. They are executed through FAPI's #element_validate property.
Example : date textfield widget - is the input a valid PHP date() string ?
- Field validation steps are common to a given field type, independently of the specific widget being used in a given form, so that every widget doesn't have to reiplement them.
Example : number field - min and max allowed values.
Field validation happens in the field type's implementation of hook_field_validate(). By the time it is performed, widgets have already done their own validation, and, if needed, massaged the form input, so hook_field_validate() operates on data in the canonical 'field value' structure (the one you get form a loaded object). So, theoretically, the field-level validation *could* be perfomed on non-form, programmatic [node|user|...]_save() - more on this later.

Now, all of this is well and cleanly separated, except that validation errors detected at field-validation time have to be flagged back to an actual form element - and hook_field_validate() knows nothing about forms, or the actual widget that was used for the input. Because CCK D6 lived under the cumulated constraints of nodeapi and formAPI, it resorted to an ugly way of having widgets communicate to hook_field_validate() the location of the form element to flag (I wont't go into this, read about '_error_field' in http://groups.drupal.org/node/18019#comment-61984 if you're interested). Having calls to form_set_error() in hook_field_validate() also forbids validation on programmatic save().

Because we don't live under nodeapi anymore, the discussion in http://groups.drupal.org/node/18019 showed a better way of handling this. The attached patch implements the following model:
- hook_field_validate() is completely form agnostic. It receives field values, and returns an array of errors for those values. An error is: an error code (machine string) + a user-facing message.
- field_attach_validate() calls the various hook_field_validate() for all fields in the entity, and raises an exception if errors are found.
- actual form validation is done in a new field_attach_form_validate() function, that calls field_attach_validate(), catches the exception if any and transfers the errors back to the widgets for error flagging.
- The latter (transferring errors back to widgets) is done through field_default_form_errors(), which calls a new hook_field_widget_error(), implemented by widgets. They receive their form $element (fully built, with the '#parents' array that corresponds to their actual location in the form) and the $error, and simply call form_error() on the sub-element that is relevant according to their own internal logic. Using the error code, complex widgets can flag different errors on different sub-elements (e.g imagefield : 'file upload' element, or 'description' element).

From the POV of a 'fieldable entity' module (node, user, ...), this simply translates to:
1) in your _form_validate handler, call field_attach_form_validate() (insetad of previously field_attach_validate) to validate the fields in your forms.
2) *if* you want your programmatic saves to be validated too, then call field_attach_validate() in your [node|user|...]_save() function, and handle the exceptions whichever way makes sense for you. If you want the same function to be called for your forms submissions, then it probably needs to accept a boolean $validate flag as a param, so that validation is not done twice for forms.

The patch doesn't take any stab at implementing 2) for current core entities. Nothing in Drupal currently has non-form validation, and changing this is far outside the scope of the patch. But it makes the field API ready if we want to go there at some point, as well as for other non-core entities that might want to do so right now.

Note :
The patch converts node_form_validate() to the new model for field validation, but does not take care of user.module yet.
The patch also changes text and number field validation to reject the value '-1', so that it can be easily tested. Obviously those changes are not supposed to be committed.
Still marking as 'code needs review' - cos' that's what it needs right now.

Exact same patch, using unix linebreaks (I can't remember if I did the conversion for the patch above)

Should field_attach_form_validate() actually modify its $object argument?
Well, not actually really useful in this specific case because the $object will most likely be ditched right after the caller returns, but it's the general pattern for all other field_attach_*() functions, and since it does no harm, we're better off not creating an exception, IMO.

I suspect that $object is a fake object anyway, created just for the purpose of calling this function. Do we even need to pass in a $object at all? ... of course, we'd still need the id, revision id, and bundle it, and they'd be separate arguments. So maybe it is best to pass it a dummy object after all.
Just like for field_attach_submit(), it's built from submitted form values. There are good ways (field_test.module) and bad ways (node.module and its $node = (object)$form_state['values']) to build it, but how it's built is not relevant to us, we need an object with basic id keys. Again, taking an object is a matter of consistency with other field_attach_*() functions. I really do like they're being so consistent :-).

Instead of just failing with a bang (== WSOD), could you set an error on the form just not connected to a specific element?
I went back and forth on this. Actually, a widget module without hook_field_widget_error() should be considered as "broken". I considered that minimal effort and 'not babysitting broken code' here means leaving the WSOD happen.
In fact, field.module could even provide a default fallback implementation of hook_field_widget_error() that would work in simple cases (like number and text fields), but I felt it would clutter field.form.inc more than anything else, plus it would be bad education since many contrib (or custom) field modules start out by deriving from those two, so it's important that they implement the hook.
Can definitely be discussed, but I'm fine with stating that the hook is "mandatory, period".

Patch rerolled.

I think the plan is right. Actually, field.test already tests the error behavior on field_test fields, so the feature itself is tested. The '-1' hack on text and number fields was just for me (and reviewers) to be able to test it visually through actual forms and actual fields.
Text and number should have their tests at some point, not sure this is a blocker for this patch. Although, stating so would sure be an incentive to start those tests rather sooner than later :-). I'd personnally plan to write more form tests first (JS 'add more', for instance...)

Hm, I forgot that TODO item :

function field_default_validate($obj_type, $object, $field, $instance, $items) {
(...)
+  // _field_invoke() does not add back items for fields not present in the
+  // original $object, so add them manually.
+  // TODO: needed ?
+  $object->{$field_name} = $items;
}

I *think* it's actually not needed anymore, but I'd need to check. Not critical, but it would probably be better to settle this before committing rather than in a followup patch.

+ * Base class for all exceptions thrown by Field API functions.
+ *
+ * This class has no functionality of its own other than allowing all
+ * Field API exceptions to be caught by a single catch block.
+ */
+class FieldException extends Exception {}

"This class has no functionality of its own" - yet CRUD exceptions still directly invoke that class, right ?

Lol, I'm not the only one mixing my patches :-)
Rerolled, plus removed the bit mentioned in #7, which indeed is not needed anymore (remnant from when field_attach_submit handled form values extraction).

So, according to #5/#6, this should be RTBC ?

right, with the patch this time.

Oh ? sorry, I was pretty sure I started off with your patch. Sorry about that.

"FieldValidationException extends FieldException" disappeared between #5 and #10 and never reappeared, along with a few doc changes.
I think I have caught all the bits and pieces now.

Currently reconsidering the if(!drupal_function_exists()) part. Not sure I'm convinced yet.
+ working on converting the user validation workflow. New patch to come.

New patch, gathers all the pieces that got lost in earlier patches, and updates field integration for user form submission workflow. On that aspect, user.module is even worse that node... Again, a user.module cleanup is outside the scope of field patches.

About hook_field_widget_error() and drupal_function_exists() (comment #21) :
Widget modules should implement the hook. -1 on saving a few lines in text and number, by embedding some fallback code in field_default_form_errors() that hardcodes 'reasonable' assumptions on internal widget structure and field-type 'column' names ($element['value']). That would be the only place in the whole field.module. + adding a comment to text.module about why some hook is *not* implemented is awkard.

The patch in #23 does the following :
If the widget fails to implement hook_field_widget_error() (and thus is *broken*), we prevent the WSOD, and simply make sure the validation error is caught, even though not visually assigned to a specific form element, as Barry suggested in #3.

Right. This issue only refactors the field-type validation workflow, without any functional change. It doesn't change the fact that the validation checks are hardcoded in the field type (often parameterized by field or instance settings). "minimum number of words" could definitely be added to text fields.

The more general feature you mention is the (long standing) 'custom validation on fields' request. So +1 on brainstorming this in a separate issue ;-)
FWIW, we outlined a possible approach in #212665: Custom validating hook for CCK, initially as a GHOP proposal, more than a year ago, and didn't make any progress since then. In a CCK world, it was targeted at UI-defined validators (validator plugins), rather than 'run my custom validation PHP code', though. Going for the latter might simplify things a little.

PS : I started to work on rehauling / generalizing $node->build_mode and the $teaser boolean param. We'll probably meet at some point !

Crap, yes, the commented out line should be removed, it's from a previous attempt. The previous line, that sets $element, can go too, it had no other purpose.

Rerolled, with the extraneous lines mentioned in #28-#29 removed.

1 fail in 'Image GD manipulation tests' - I don't think this is relevant to our patch... Plz retry, bot.

Needed a reroll after a minor comment change in HEAD.
I *really* think this is ready now...

"I also created a node programmatically and tried to save it using node_save() with an invalid value and it correctly stripped out the value I tried to save"
Are you sure about this ? From my own tests, 'invalid' values are *not* filtered out on programmatic save - and I really think we should leave it that way in this patch. From my comment #2 above :
"The patch doesn't take any stab at implementing [validation on programmatic save] for current core entities. Nothing in Drupal currently has non-form validation, and changing this is far outside the scope of the patch. But it makes the field API ready if we want to go there at some point, as well as for other non-core entities that might want to do so right now."

Patch missed one function call in _field_attach_validate() :

    $function = $module . '_field_attach_validate';
    $function($obj_type, $object, $form);
    $function($obj_type, $object, $errors);

Attached patch removes the first call (deprecated). Pretty obvious, so I'm setting straight to RTBC.

Webchick: I don't think I see where documentation is still needed : the PHPdoc for hook_field_validate() is updated, and field_attach_validate() / field_attach_form_validate() have reasonably extensive docs. What other place are you thinking of ?

Right, there's no such page currently, and there should definitely be one sooner rather than later...
Created #419348: Handbook for fields API