document Form API #element_validate callback

I'll have a shot at this one. Patch soon.

Here's a first run at it. I've added callback_form_element_validate() in a new file, form.api.php.

• Really not sure what this is a callback for, though I've put hook_form().
• Need to add some docs referring back to callback_form_element_validate() from where ever point one decides on.
• Wasn't quite sure about a couple of '#element_validate' calls within classes, and whether they need some different docs.
• If the first line on a doc had some value, I've kept it as the first line of the second docblock. I figure its better to keep detail instead of removing it. Most instances I've simply replaced the first line.

What to improve?

Cheers,
Sam.

Thanks, updated with the suggested changes.

I added back in most of the original first rows as the second rows.

Cheers,
Sam.

Fix for missing paren.

Thanks for the patches!

A few things to address before this is ready for commit:

a) When I look at this patch, I see a nice standardization of the first line of these callback implementing functions. But maybe we can make it even better? What if the first line instead said:

Implements callback_form_element_validate() for a foo_bar element.

or maybe just

Implements callback_form_element_validate() for a foo_bar.
(especially if the first form wouldn't fit all in one line).

Then we could eliminate the second line (which isn't at all standard), and also the first line (which is what appears in function lists and the like) would give us more information. We use standards like that in some hook implementations, such as "Implements hook_form_FORM_ID_alter for the user login form".

Thoughts?

b) If for some reason you think (a) is a bad idea, then when you do something like this:

+ * Form element validation handler for #type 'email'.
  * Note that #maxlength and #required is validated by _form_validate() already.
  */
 function form_validate_email(&$element, &$form_state) {

you either need to make this into two separate paragraphs, or wrap the two sentences together as a single paragraph.

c) In the callback definition itself, do not use @addtogroup callbacks. You have @ingroup callbacks already in the function. And please check to verify that this is not inside the @addtogroup hooks that is in system.api.php.

d) Do not indent the entire callback definition function two spaces inside system.api.php. It should be over to the left.

e) This line in the callback definition:

+   *  @param array $element

has an extra space of indentation between the * and the @param

f) The documentation for this parameter:

+   *   An associative array containing the properties and children of the
+   *   table element.

It should not say "the table element", but just "the element".

g) At the end of that doc block:

+   * @ingroup callbacks
+   *
+   */
+  function callback_form_element_validate($element, $form_state) {

Remove the blank line.

h) Add an explanation in the documentation of the example callback like "The example function body is for validating a ..." (I am actually not sure what it is?).

i) This implementation did not get the standard first line:

+++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -454,6 +454,8 @@ function form_test_validate_form($form, &$form_state) {
 
 /**
  * Form validation handler for form_test_validate_form().
+ *
+ * Form element validation handler for the 'test' element.
  */
 function form_test_validate_form_validate(&$form, &$form_state) {

Hey, thanks for the detailed review!

I've taken all these comments on board in the attached patch. A couple of questions/comments:

1. Is this the way to use @see in a class?

* @see \Drupal\rest\Plugin\views\row\DataFieldRow::buildOptionsForm().

2. There are several of these validate functions that mention they are 'Render API callbacks'. Should we keep these lines even though they effectively duplicate the content of the first line?

3. I switched the example in the api.php file to the password compare code from form.inc.

Cheers,
Sam.

Regarding your questions:

1. Yes, that usage of @see is fine. You can also just do @see DataFieldRow::buildOptionsForm(). The rule is that if you use a namespace, it must start with a \, but you don't have to use a namespace if it's already referenced by the file in a namespace/use statement.

2. I don't think we need to include the "Render API callback" information. Also, we don't need the @param/@return if you say "Implements callback_foo()" (it's like implements hook_foo() -- we reference the definition already given in the callback definition). The only reason to have @param/@return is if it gives some information specific to that particular callback, but in that case it can probably be put in a separate paragraph anyway, like you could say about some validate callback "Verifies that the email address conforms to RFC ### and that the domain name is not in a list of spammers this module maintains.".

3. Good idea. :)

So, regarding the current patch... Besides removing param/return as noted in 2. above, I think it would also be nice if all of the "Implements ... for ..." statements were a little more consistent. Some say "for a foo_bar element", some say "for a #foo_bar element" [this is actually wrong, we don't put # before element names!], some say "for a foo bar element", some say "for #type 'foo_bar'", etc.

So let's standardize on one of these... My preference would be either:
- Implements callback_form_element_validate() for a foo_bar element.
- Implements callback_form_element_validate() for #type 'foo_bar'.
Your choice... or you can come up with something better, but let's try to keep them all the same?

Hm... I see that some of them are "elements" and some are "widgets". In the case of widgets, possibly they are not really being used as #element_validate callbacks? Check on that -- it's possible they are a different callback from the field API? If so we should file a separate issue and document that callback.

I also think that anything that's a class member is probably not really a callback_form_element_validate(), but again it's probably something else? But maybe I'm wrong... The idea here is that something should only be documented as "Implements callback_form_element_validate()" if it is being put into a form array as a #element_validate property. Right?

Oh, and one more thing. The callback function doc says it is a callback for drupal_get_form(). But unless drupal_get_form() or the Form API reference is going to mention callback_form_element_validate(), I think we need to say more than "Callback for drupal_get_form()" here. How about something like:

Provided as the #element_validate property for an element of a form array for drupal_get_form().

or something like that?

All good points, but before i roll the next patch I want to clarify a bit.

There are several types of #element_validate functions in core:

1. Generic form elements as defined by Form API.
2. Field API callbacks that are defined in the #element_validate (these are widget forms and element forms)
3. Validate callbacks for various admin forms.
4. Validate elements applied to a whole form.
5. Class based callbacks.

Type 1 are obvious inclusions for this patch and fit the "Implements callback_form_element_validate() for #type 'foo_bar'." doc perfectly.

Type 2 are out of scope for this patch as they are not defined in forms, they are defined in hook_element_info() or hook_field_instance_settings_form(). However hook_field_instance_settings_form() is actually a form so perhaps our doc does apply here. Hook_element_info element validate functions should be in 'callback_element_info_validate()'.

Type 3 should be in scope, however the docs will be "Implements callback_form_element_validate() for #type 'textfield'.", which may be confusing as it is actually for a specific implementation of a textfield. Do we need some way to indicate this?

Type 4 has a similar problem to type 3 - it isn't for a specific #type.

Type 5 class methods should probably be documented in the class i guess.

All of these apart from the hook_element_info() ones are using the #element_validate key from a form definition.

What do you think?

This issue is supposed to be about documenting the callbacks for the Form API's #element_validate.... I think only (1) from the list in comment #11 falls into the scope of this issue, but I'm not sure... Can you provide some examples of each one so we can decide? And please do not change the status to "needs review" until we have another patch to review. Thanks!

@joachim, I believe all the included functions are actually #element_validate related: thats how i found them in the first place.

Examples of each type:

1. Initial element level fields, such as url, number, color, machine_name. These are definitely in scope, and are all quite similar in their execution. Here is an example using the email element.

function system_element_info() {
  ....
  $types['email'] = array(
    '#input' => TRUE,
    '#size' => 60,
    // user.module is not loaded in case of early bootstrap errors.
    '#maxlength' => defined('EMAIL_MAX_LENGTH') ? EMAIL_MAX_LENGTH : 255,
    '#autocomplete_path' => FALSE,
    '#process' => array('form_process_autocomplete', 'ajax_process_form', 'form_process_pattern'),
    '#element_validate' => array('form_validate_email'),}
   ...
}

/**
 * Implements callback_form_element_validate() for #type 'email'.
 *
 * Note that #maxlength and #required are validated by _form_validate() already.
 */
function form_validate_email(&$element, &$form_state) {
  $value = trim($element['#value']);
  form_set_value($element, $value, $form_state);

  if ($value !== '' && !valid_email_address($value)) {
    form_error($element, t('The e-mail address %mail is not valid.', array('%mail' => $value)));
  }
}

2. Field API Callbacks - These don't exist anymore as Fields are now Plugins! These are now type 5.

3. Validate callbacks for forms. With this example, the #type of the element is actually textfield, but putting that in the comment isn't accurate because it's not for generic textfield elements, it really relates to a specific instance of the textfield element, 'bgcolor'. The documentation may be more useful if it makes this clear. From image_rotate_form():

$form['bgcolor'] = array(
    '#type' => 'textfield',
    '#default_value' => (isset($data['bgcolor'])) ? $data['bgcolor'] : '#FFFFFF',
    '#title' => t('Background color'),
    '#description' => t('The background color to use for exposed areas of the image. Use web-style hex colors (#FFFFFF for white, #000000 for black). Leave blank for transparency on image types that support it.'),
    '#size' => 7,
    '#maxlength' => 7,
    '#element_validate' => array('image_effect_color_validate'),
  );

/**
 * Implements callback_form_element_validate() for #type 'textfield'.
 *
 * Ensures a hexadecimal color value.
 */
function image_effect_color_validate($element, &$form_state) {
  if ($element['#value'] != '') {
    $hex_value = preg_replace('/^#/', '', $element['#value']);
    if (!preg_match('/^#[0-9A-F]{3}([0-9A-F]{3})?$/', $element['#value'])) {
      form_error($element, t('!name must be a hexadecimal color value.', array('!name' => $element['#title'])));
    }
  }
}

4. Validate callbacks applied to whole forms. Disregard this, I must have been drinking. This isn't even legal according to the FAPI docs.

5. Class Based Callbacks - this example is found inside the LinkWidget field plugin class. Callback docs may not make sense here, perhaps it should be referencing the parent class's method? There's a bunch of the in various plugin definitions and I'm sure there are more coming.

class LinkWidget extends WidgetBase {

  /**
   * {@inheritdoc}
   */
  public function formElement(array $items, $delta, array $element, $langcode, array &$form, array &$form_state) {
    $instance = $this->instance;
    ...
    // Post-process the title field to make it conditionally required if URL is
    // non-empty. Omit the validation on the field edit form, since the field
    // settings cannot be saved otherwise.
    $is_field_edit_form = ($element['#entity'] === NULL);
    if (!$is_field_edit_form && $instance['settings']['title'] == DRUPAL_REQUIRED) {
      $element['#element_validate'] = array(array($this, 'validateTitle'));
    }
   ...
  }
  
  /**
   * Implements callback_form_element_validate() for link widget title elements.
   *
   * Conditionally requires the link title if a URL value was filled in.
   *
   * @see link_field_widget_form()
   */
  function validateTitle(&$element, &$form_state, $form) {
    if ($element['url']['#value'] !== '' && $element['title']['#value'] === '') {
      $element['title']['#required'] = TRUE;
      form_error($element['title'], t('!name field is required.', array('!name' => $element['title']['#title'])));
    }
  }
}

I don't see a problem in defining all of the examples in #15 as "Implements callback_form_element_validate() for ...". Here is what I would suggest for the first line in each case:

1) Implements callback_form_element_validate() for #type 'color' elements.
3) Implements callback_form_element_validate() for background color elements.
5) Implements callback_form_element_validate() for the widget's title element.

Does that make sense? I think it gets across in each case what type of data is being validated. Also, somewhere in the function doc, probably there should be an @see to the form generating function or system_element_info() or wherever the validate callback is used. Except I don't think it's necessary in case 5 since this is inside the LinkWidget class. (Also, as a note: I don't think there should be * @see link_field_widget_form() in that last example??)

Cool sounds good. Only thing I am concerned about is the 80 char limit. I was trying to do something similar in one of the earlier patches and needed to deviate from the pattern to fit into 80. Is there any leeway here or should I definitely keep the first line under 80 chars. The docs seem to say it is a hard rule.

The 80 character limit is a hard rule for any documentation line except an @link...@endlink or within @code...@endcode. I think you'll be OK though.

Just a question about this kind of construct:

      '#element_validate' => array(
        array($this, 'validate'),
      ),

What can I substitute for $this? If it's the name of a class, does it have to be static or does FAPI instantiate one? Do I have to instantiate one, store it on $this and then say array($this->validatorobject, 'validate'),?

Sure would be nice for this patch to see the light of day... :-)

I still feel this a novice task, but some issue summary work is needed to pull together a lot of conversations and get a fresh patch rolled.

Un-assigning so others feel like they can work on this.

Working on this

Hello
Unassigning alvar0hurtad0 as seen in IRC.
I'll work on it today

I'm working with Marabak on this issue.

At the moment, the patch in #9 does not apply anymore and most of documentations in the patch doesn't not seem to be relevant anymore. Comments are related to hooks, hooks_alters, callbacks... Now everything have moved to classes, the documentation should be rewritten.

Here is a patch in order to update validate methods to several form elements.

This patch needs to be continued. It seems there are functions that doesn't exist anymore but I'm not capable to says if it's because I haven't found them or if they have really been removed

We don't have a standard for methods vs. functions for callbacks, but I would think we could still document the method signature as a callback function in an api.php file. After all, a method *is* a function.

Setting to needs review to test the patch in #27.

For me the difference between the methods and functions is the ability to use @inheritdoc? That would suggest that if we're trying to propogate documentation then we would use @inheritdoc to do so for methods where the inheritance chain suggests good docs.

From my perspective the choices should be either an @see for relavent centralized documentation or @inheritdoc when it makes sense or repeat the documentation when there is no good documentaition. Am I missing something here?

OK... Here is what I think we should do:

a) If a base class or interface has the callback method on it, I think we should document that method on the base class/interface using our standards for how to document callbacks in api.php files shown here:
https://www.drupal.org/node/1354#callback-def

And then in the extending class, we would just use @inheritdoc for that method.

b) If the base class or interface does not have the callback method on it, then we should document the function in a *.api.php file following the standards on
https://www.drupal.org/node/1354#callback-def
and then the method would be documented using the "Implements callback_foo()" line shown there.

Does that make sense?

For cross-referencing purposes, the change record is Form API now allows for object methods as callbacks.

Adding tag for sprints.

Tagging. The patch is adding missing @param documentation for this callback throughout core.

The patch in #27 needs a reroll. I think this is suitable for a novice, leaving the tag.

When making the reroll remember to help reviewers add an interdiff, or a diff, whichever is appropriate. There are instructions for creating an interdiff .

Working on it

Re-rolled patch against 9.4.x. Please review

Fixed custom command failure

Re-rolled patch against #51 in 9.5.x

Re-rolled patch against #54 in 10.0.x.
Please review.

I have fixed CS error. Please review it

Reroll the patch #57 with Drupal 10.1.x

Re-rolled patch against #57 in 10.1.x-dev
Please review it.

Thank you

The patches in #57 and #58 are the exact same file. #59 is a reroll with no changes.

Use the "Add test / retest" link to see if it needs a reroll.

Seems the issue summary update has not happened yet.

and in #52 it was noted this needs work also

Just FYI to help get the message out there.

Starting March 2023, simple rerolls, rebases, or merges will no longer receive issue credit. Only rerolls that address a merge conflict will be credited, and the merge conflict that was resolved must be documented in the text of an issue comment.
To receive credit for contributing to this issue, assist with other outstanding tasks or unaddressed feedback.
See the issue credit guidelines for more information.