image_button: clicked_button value is wrong

You can't reliably get the value of an image button. But you don't need to; instead you can get the actual form element info of the image button, and you can store something in there to get you whatever value you need. That's actually better, though obviously it'll require some code rework.

Hass, I am trying to understand what is going on here, but from the dump you have above in the report, you have:

    ["#has_garbage_value"]=> bool(true)
    ["#value"]=> string(12) "twocol_75_25"

So empty($form['#has_garbage_value']) && isset($form['#value']) && $form['#value'] !== '' should be TRUE, and then _form_button_was_clicked() will return TRUE. But this goes through all #input types (from form_builder() to _form_builder_handle_input_element() and then to _form_button_was_clicked(). So you get the last button as a result.

Looks like $form_state['values'] is only === TRUE for the button clicked, for the others, it is a string, so the button is rightly identified. It is just not properly put into clicked_button.

You have a #default_value

If you have a #default_value and no value is received in $_POST, that #default_value is moved to #value. That makes it appear that the button was clicked.

Remove the #default_value.

Looks like this is by-design. Reopen if merlinofchaos' tip did not solve the problem.

Whoops, that wasn't supposed to happen.

Try this patch. It prevents #default_value propagation if #has_garbage_value is set.

It's not safe to give image buttons values because of IE. That said, the patch I presented *should* prevent unexpected propagation of the default value to the value. But even without the patch, omit the default_value and, as I suggested very early on, use something like '#layout' to get the value out of clicked button.

To explain what is happening:

The process that checks whether or not the button is clicked is testing to see if a value was returned for the button. However, because you have a #default_value, a #value is being set even though there is no value in $_POST.

Therefore, the form thinks all buttons were clicked, thus you see the last button.

Now, the patch supplied prevents #value from getting set to #default_value if #has_garbage_value is set. I'm actually not completely sure that's the right way of doing that, but I'm not sure it's the wrong way either.

hass: why do you need a value at all? If it is pressed, it will be your clicked_button, if not, then not. What purpose would whatever value serve?

BTW my patch exists less to fix hass' problem than it does to prevent broken behavior when #default_value is used together with image buttons. I think it's safe to say that more than one user will set #default_value on an image button to try and get a value out there, and then be confused as to why that button (or worse, some other button *also* with #default_value set) is always showing up as clicked.

maybe this is related http://drupal.org/node/209574
the last patch is missing one parenthesis, and does not solved my problem..

Hass, #17 still looks like having a parse error, so I doubt you tested the patch.

Thanks, committed with this added documentation:

      // Avoid image buttons (which come with garbage value), so we only get value
      // for the button actually clicked.

It looks like this is still a bug in 6.2 if a #value attribute is filled in.

Here is some test code to generate the error, if you remove the #value attribute under the image button everything works:

test_form.info

name = Update Test Form
description = Module for Updating the Test Form
core = 6.x

test_form.module

function test_form_menu() {
  $items['test_form/my_test'] = array(
    'title' => t('Test Form'),
    'page callback' => 'gen_test_form',
    'access arguments' => array('access test_gen'),
    'description' => t('Test Form'),
    'type' => MENU_CALLBACK,
  );
  return $items;
}

function gen_test_form() {
  $content = drupal_get_form('test_form');
  return $content;
  
}
   
function test_form($form_state) {
  // Access log settings:
  $form['#theme'] = 'test_form_setup';
  $rows_per_page = 50;
	$header = array(
		array('data' => t('Delete'))
	);
  
	for ( $counter = 0; $counter <= 5; $counter++) {
	    $form['delete'][$counter] = array(
			//'#type' => 'submit',
			'#type' => 'image_button',
			'#value' => 'Submit',
			'#src' => 'sites/default/files/images/delete.png',
			'#name' => 'delete'.$counter,
			'#submit' => array('test_form__delete_submit'),
		);
   }
   
   $form['header'] = array(
	    '#type' => 'value',
	    '#value' => $header
	  );
  
  return $form;
}

function test_form__delete_submit($form, &$form_state)
{
	drupal_set_message($form_state['clicked_button']['#name']." Button pressed");
}

function theme_test_form_setup(

$form) {
	$rows = array();
	foreach (element_children($form['delete']) as $key) {
	    $row = array();
	    $row['data'][0] = drupal_render($form['delete'][$key]);
	    $rows[] = $row;
	}
	
	$output = theme('table', $form['header']['#value'], $rows);
	$output .= drupal_render($form);
	return $output;
}

function test_form_theme()
{
	return array(
		'test_form_setup' => array(
			'arguments' => array('form' => NULL),
			),
	);
}

earnie told me that this was supposed to be fixed in 6.2, but as you can see from my example it is not fixed. I'm referring to the #value attribute, not the #default_value attribute in this example, but the bug is the same. If you don't use the #value attribute, everything works as expected, but if you supply a #value then it fails. Obviously, this isn't a big bug, but it's very confusing to try and debug.

confirmed. when putting garbage into #value the image_button handling fails..

simplified test attached

This is similar to http://drupal.org/node/359529 you might want to set it as a duplicate entry

Marked #359529: _form_button_was_clicked returns the wrong value as a duplicate.

Having just fought with this for a while, and not having found clear documentation, I'm adding what seem like the key points for using multiple image_buttons in a form:

1) Do give each image_button a unique #name (unlike regular buttons, you can't have several image_buttons all named op).
2) Don't give an image_button a #default_value.
3) Don't give an image_button a #value
4) Do give each image_button a #return_value. This is the same as what you would assign to #value if IE6 problems hadn't forced Drupal to implement some unusual element handling. If you don't assign this, it will default to true.
5) Do use $form_state['clicked_button']['#value'] to find the #value of, well, the clicked image_button. This field will contain the value of #return_value for the clicked image_button.

I had to create a table, with at the end of each row, 3 buttons. Each button changed the status of the item either to approved, disapproved, or on hold.

After reading this post, I did this. Don't know if it's really the way to go, but it works :)

        $form['approve'][$records->id] = array(
          '#name' => 'approve_' . $records->id,
          '#type' => 'image_button',
          '#submit' => array('sales_overview_status_submit'),
          '#src' => path_to_theme() . '/images/approve.gif',
          '#attributes' => array('class' => 'approve'),
          '#return_value' => 'approve_' . $records->id,
        );
        $form['disapprove'][$records->id] = array(
          '#name' => 'disapprove_' . $records->id,        
          '#type' => 'image_button',
          '#submit' => array('sales_overview_status_submit'),
          '#src' => path_to_theme() . '/images/disapprove.gif',
          '#attributes' => array('class' => 'disapprove'), 
          '#return_value' => 'disapprove_' . $records->id,                                       
        );
        $form['onhold'][$records->id] = array(
          '#name' => 'onhold_' . $records->id,        
          '#type' => 'image_button',
          '#submit' => array('sales_overview_status_submit'),
          '#src' => path_to_theme() . '/images/onhold.gif', 
          '#attributes' => array('class' => 'onhold'), 
          '#return_value' => 'onhold_' . $records->id,                           
        ); 

3 buttons per row, each with a unique name, and as return_value the action name + the record id.

I then did this in the submit :

function sales_overview_status_submit($form_id, $form_values) {

    $button = $form_values['clicked_button']['#value'];
    $values = explode('_',$button);
    
    list($action, $record) = $values;
    
    drupal_set_message($action . ' - ' . $record);    
}

And now I can do the $action, based on the $record I got.

vodde83,
this is similar to what I did but notice that #name is not a publicly available property. Also note that #return_value is not documented for image_button.

heine (http://drupal.org/user/17943) suggested I use the tree property when dealing with arrays. I did this successfully. Your explode logic is an analogy to an enhancement I suggested where tree named button names should be parsed by Drupal

See my issue http://drupal.org/node/362249

@#34: In my case it is enough to just multiple image buttons as follows:

  $form['button1'] = array(
    '#type' => 'image_button',
    '#src' => 'url/button1.gif',
  );

  $form['button2'] = array(
    '#type' => 'image_button',
    '#src' => 'url/button2.gif',
  );

If I add #value, clicked_button always returns the last button in the form.

I'm not sure what the expected status of this is at the moment, but I tried converting the two buttons on the cart form for ubercart over to image_buttons, and no matter what I did, it kept saying the clicked_button was the second one (checkout). I tried setting them to using different #submit arrays, and even still it would only call the #submit functions for the checkout button.

The information in #34 works perfectly.

If this issue is not going to be patched, at the very least it would be nice to see a rewording of the documentation of image_button in then Forms API Reference to contain the information in #34 (and an example).

using #name worked here. Thank you.

I had exactly the same issue with the ubercart buttons, and I tried following #34 but it didn't work - then I realized the issue is that you need to enforce those "do not's" from #34 - in my case the "do not set a #value for an image button". What I mean is, when altering a form, just because you don't set the "#value" field, doesn't mean it isn't already set. Thus in my case, I had to run unset($form['field_name']['#value']) on each of the image button fields.

All of these operations are of course counter-intuitive and really should be fixed.

I just ran into this problem in Drupal 7. The points in #34 were a great help. I only needed to set #return_value instead of #value. It seems like D7 automatically gives different names to different image buttons.

If this is going to be improved/fixed, we need to do it in 8.x-dev first and then backport to earlier versions.

Thanks again for #34.

Subscribing.

The issue still persists in Drupal 7 if image buttons are used through AJAX calls. I isolated the problem in function _form_button_was_clicked in file form.inc/2031 where the elseif is true for all image buttons that carry a value. I believe if this code is used (in line 2045):

  elseif (!empty($element['#has_garbage_value']) && isset($element['#value']) && $element['#value'] !== '') {
	if (!empty($form_state['input']['_triggering_element_name']) && $element['#name'] == $form_state['input']['_triggering_element_name']) {
		return TRUE;
	}
  }

instead of:

  elseif (!empty($element['#has_garbage_value']) && isset($element['#value']) && $element['#value'] !== '') {
	return TRUE;
  }

then the issue will be fixed.

I think the remaining work here is covered by other issues (both of which already have patches):
#873070: When an image button appears after another button in a form, the wrong triggering element and #submit handlers are detected
#1452894: Elements with #has_garbage_value and #value are always set as a triggering element

So, tentatively moving this back to Drupal 6 and closing.