Currently, most weight elements do not have a default value defined, and as such, the weight value selected by default on most forms is the one with the lowest value (e.g. -10 for taxonomy forms, -15 for book page forms, etc). Weight elements should be having their default value set to zero. This is what was happening before the forms API - but since the forms API came in, it is no longer happening. But to set it manually for every weight element in core would be a hassle, and is not the 'best' way to solve the problem.

The attached patch implements the _value hook in the forms API, for the weight element. It uses the default value if one is given; otherwise, it uses a default value of zero (0). When I apply this patch, all weight form elements once again correctly default to zero.

CommentFileSizeAuthor
#5 form_weight_default_2.patch711 bytesJaza
#2 form_weight_default_1.patch653 byteschx
#1 form_weight_default_0.patch711 bytesJaza
form_weight_default.patch798 bytesJaza
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jaza’s picture

Jaza CreditAttribution: Jaza commented
FileSize
form_weight_default_0.patch711 bytes

Whoops.. the "if (!isset($form['#value']))" check is not needed, because this check is made before the _value fAPI hook can be invoked.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community
FileSize
form_weight_default_1.patch653 bytes

The _value is needed when you need a dynamic value. But solving this problem does not need any code.

Jaza’s picture

Jaza CreditAttribution: Jaza commented

Ahh.. so that's where you define defaults for each form element. Cool! (That also points the way to module-defined form elements, should I ever need to go down that road...).

[insert +1 here] :-)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Alrighty! Committed to HEAD. Thanks folks.

Jaza’s picture

Jaza CreditAttribution: Jaza commented
Status: Fixed » Reviewed & tested by the community
FileSize
form_weight_default_2.patch711 bytes

Still experiencing this problem, guys. The patch that just got committed (by chx) was needed, and would do the job IF most forms simply left '#default_value' undefined. But most forms set #default_value to NULL (e.g. the 'add/edit vocabulary' form sets '#default_value' for vocabulary weights to $edit['weight'] - but when it's a new vocab, $edit['weight'] is not set, so '#default_value' becomes NULL).

Then when drupal_get_form() gets to here:

$form = array_merge(_element_info('form'), $form);

It merges the '#default_value' provided by hook_elements() with that provided by the actual form. And since $form has already defined '#default_value' for its weight elements (i.e. it's set them to NULL), the value provided by hook_elements() (which is now zero) never gets used.

Attached is my original patch (rerolled for latest HEAD), which still fixes the problem for virtually all forms in core that use a weight element. The only other alternative is to change the way that defaults are overridden by form values (i.e. use a loop instead of array_merge), but this is bound to significantly affect performance. So I think this patch is realistically the best solution to the problem.

Dries’s picture

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

There is no call to the function?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Reviewed & tested by the community 
      if (!isset($form['#value'])) {
        $function = $form['#type'] . '_value';
        if (function_exists($function)) {
          $function($form);
        }
        else {
          $form['#value'] = $form['#default_value'];
        }

second line. form.inc calls everything indirectly.

markus_petrux’s picture

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

Wow, these _value hooks are gold! :-)

Just a comment, what if weight is set, but it is not numeric?

function weight_value(&$form) {
  if (is_numeric($form['#default_value'])) {
    $form['#value'] = $form['#default_value'];
  }
  else {
    $form['#value'] = 0;
  }
}
Jaza’s picture

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

Umm.. the weight is always numeric, it's a required select element that has only numeric options. All we need to be checking is that it's not NULL (or undefined), and isset() is the most efficient way to do this.

Dries’s picture

Dries CreditAttribution: Dries commented

Ah, got it now. Does that mean we can undo the previous patch? (Any dead code?)

Jaza’s picture

Jaza CreditAttribution: Jaza commented

No - the two patches both cover different use cases, and are therefore both needed.

Use case 1, covered by chx's patch (already committed):

$form['weight'] = array('#type' => 'weight', '#delta' => 10, 'title' => t('Weight'));

Use case 2, covered by my patch (not yet committed):

$form['weight'] = array('#type' => 'weight', '#delta' => 10, 'title' => t('Weight'), '#default_value' => $edit['value_that_is_sometimes_null']);

Use case 2 is more common in Drupal core - which is why this patch needs to go in. ;-)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks for the explanation.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)