Zend Framework

Zend_Dojo_Form Checkbox element generates incorrect HTML

Details

  • Type: Patch Patch
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.0
  • Fix Version/s: None
  • Component/s: Zend_Dojo
  • Labels:
    None

Description

Zend_Dojo_Form checkbox generates incorrect HTML. Assuming a working Zend_Dojo environment, the following code produces the error:

"TestController.php"
class TestController extends Zend_Controller_Action {


	function indexAction () {
		
		$this->view->addHelperPath('Zend/Dojo/View/Helper/', 'Zend_Dojo_View_Helper');
		$form = new Zend_Dojo_Form();
		$form->addElement(
		    'CheckBox', 
		    'checkboxValue', 
		    array(
		        'label'        => 'Label',
		        'checkedValue' => 'checkedValue',
			'uncheckedValue' => 'notCheckedValue',
		    )
		);

		$form->addDecorators(array('FormElements', 'Form'));
		$this->view->form = $form;
	}
}

This produces the following HTML:

<input name="checkboxValue" value="0" type="hidden"><input id="checkboxValue" name="checkboxValue" value="notCheckedValue" type="checkbox">

The correct HTML should be:

<input name="checkboxValue" value="notCheckedValue" type="hidden"><input id="checkboxValue" name="checkboxValue" value="checkedValue" type="checkbox">

Tested against SVN Trunk r 11380.

  1. DijitElement-ZF-4274.patch
    07/Nov/08 3:32 AM
    1 kB
    Bart McLeod
  2. DijitElement-ZF-4274.patch
    06/Nov/08 2:51 PM
    1 kB
    Bart McLeod
  3. DijitElement-ZF-4274.patch
    06/Nov/08 2:20 PM
    6 kB
    Bart McLeod
  4. DijitElement-ZF-4274-3.patch
    18/Nov/08 5:23 AM
    0.9 kB
    Bart McLeod
  5. DijitElement-ZF-4274-4.patch
    18/Nov/08 6:11 AM
    1 kB
    Bart McLeod
  6. DijitElement-ZF-4274-test.patch
    06/Nov/08 2:53 PM
    1 kB
    Bart McLeod

Activity

Hide
Andrew Yager added a comment -

This is the wrong fix, but it is a fix. The problem is that the $checkedOptions parameter is not set when passed to the Helper.

--- Zend/Dojo/View/Helper/CheckBox.php	(revision 11380)
+++ Zend/Dojo/View/Helper/CheckBox.php	(working copy)

 /** Zend_Dojo_View_Helper_Dijit */
@@ -72,7 +72,11 @@
         } elseif (isset($attribs['checked'])) {
             $checked = false;
         }
+
+	$checkedOptions = $attribs['options'];
+
         $checkboxInfo = Zend_View_Helper_FormCheckbox::determineCheckboxInfo($value, $checked, $checkedOptions);
         $attribs['checked'] = $checkboxInfo['checked'];
         if (!array_key_exists('id', $attribs)) {
             $attribs['id'] = $id;
Show
Andrew Yager added a comment - This is the wrong fix, but it is a fix. The problem is that the $checkedOptions parameter is not set when passed to the Helper.
--- Zend/Dojo/View/Helper/CheckBox.php	(revision 11380)
+++ Zend/Dojo/View/Helper/CheckBox.php	(working copy)

 /** Zend_Dojo_View_Helper_Dijit */
@@ -72,7 +72,11 @@
         } elseif (isset($attribs['checked'])) {
             $checked = false;
         }
+
+	$checkedOptions = $attribs['options'];
+
         $checkboxInfo = Zend_View_Helper_FormCheckbox::determineCheckboxInfo($value, $checked, $checkedOptions);
         $attribs['checked'] = $checkboxInfo['checked'];
         if (!array_key_exists('id', $attribs)) {
             $attribs['id'] = $id;
Hide
Benjamin Jeanjean added a comment -

This component is unusable in it's actual state... It don't work at all !

Show
Benjamin Jeanjean added a comment - This component is unusable in it's actual state... It don't work at all !
Hide
Bernd Matzner added a comment -

In addition, the view helper adds

checked="checked"
even if the form element value is set to the unChecked value. I assume this is so because the value of the hidden field differs from the unchecked value, because of which it is incorrectly assumed that the field should thus be checked.

Show
Bernd Matzner added a comment - In addition, the view helper adds
checked="checked"
even if the form element value is set to the unChecked value. I assume this is so because the value of the hidden field differs from the unchecked value, because of which it is incorrectly assumed that the field should thus be checked.
Hide
Bart McLeod added a comment -

The problem was the options set in FormCheckbox.php were left unused.

We merge with existing options, because options may already be in use by multioptions.

This is a risk, so we must further test if this wil work with a checkbox group that has multioptions.

This code is added in DijitElement::render():

if(array_key_exists('options', $attribs)){
        	if(is_array($options)){
        		$options = array_merge($options, $attribs['options']);
        	}else{
        		$options = $attribs['options'];
        	}
        }
Show
Bart McLeod added a comment - The problem was the options set in FormCheckbox.php were left unused. We merge with existing options, because options may already be in use by multioptions. This is a risk, so we must further test if this wil work with a checkbox group that has multioptions. This code is added in DijitElement::render():
if(array_key_exists('options', $attribs)){
        	if(is_array($options)){
        		$options = array_merge($options, $attribs['options']);
        	}else{
        		$options = $attribs['options'];
        	}
        }
Hide
Bart McLeod added a comment -

trying to overwrite wrong patch file (is whole file instead of patch)

Show
Bart McLeod added a comment - trying to overwrite wrong patch file (is whole file instead of patch)
Hide
Bart McLeod added a comment -

Patch for tests/Zend/Dojo/Form/Element/CheckboxTest.php

Show
Bart McLeod added a comment - Patch for tests/Zend/Dojo/Form/Element/CheckboxTest.php
Hide
Bart McLeod added a comment -

Do not use the last patch in the list (the oldest). It is the whole file (sorry). It's size is 6kb, you should use the 2kb version that is listed one higher up.

Show
Bart McLeod added a comment - Do not use the last patch in the list (the oldest). It is the whole file (sorry). It's size is 6kb, you should use the 2kb version that is listed one higher up.
Hide
Bart McLeod added a comment -

Please review. I think it is quite allright, but you should take a look at how checkboxgroups behave in Dojo after patching. I should write a test for that myself, but I need to get some sleep.

Show
Bart McLeod added a comment - Please review. I think it is quite allright, but you should take a look at how checkboxgroups behave in Dojo after patching. I should write a test for that myself, but I need to get some sleep.
Hide
Bart McLeod added a comment -

Found that the test patch and the dijit patch are the same: they both are the testpatch. So I try once again and upload the decorator/DijitElement.php patch.

Show
Bart McLeod added a comment - Found that the test patch and the dijit patch are the same: they both are the testpatch. So I try once again and upload the decorator/DijitElement.php patch.
Hide
Bart McLeod added a comment -

better batch, leaves multioptions intact instead of doubling them by using array_merge

Show
Bart McLeod added a comment - better batch, leaves multioptions intact instead of doubling them by using array_merge
Hide
Bart McLeod added a comment -

Better patch (number 4) deletes the lines where getMultiOptions() is called internally. This line is no longer necessary and existing tests still pass.

Also a manual test proved that these lines were no longer needed.

Show
Bart McLeod added a comment - Better patch (number 4) deletes the lines where getMultiOptions() is called internally. This line is no longer necessary and existing tests still pass. Also a manual test proved that these lines were no longer needed.
Hide
Bart McLeod added a comment -

Committed to svn after contacting Matthew about this and resolving the conflict with multioptions.

Show
Bart McLeod added a comment - Committed to svn after contacting Matthew about this and resolving the conflict with multioptions.
Hide
Bart McLeod added a comment -

affects version 1.7 and prior.

Show
Bart McLeod added a comment - affects version 1.7 and prior.

People

Vote (9)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved:

Time Tracking

Estimated:
Not Specified
Original Estimate - Not Specified
Remaining:
0m
Remaining Estimate - 0 minutes
Logged:
3h 32m
Time Spent - 3 hours, 32 minutes