Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

incomplete regression fix for FormHelper selection - this resolves it for good #1105

Merged
merged 1 commit into from Feb 7, 2013

Conversation

Projects
None yet
3 participants
Member

dereuromark commented Feb 1, 2013

As suspected due to my local tests with in_array() our fix for master and 2.2 is incomplete!

$options = array(1 => 'One', 2 => 'Two', 3 => 'Three', '3x' => 'Stringy');
$disabled = array('2', '3x');

would still disable three fields instead of two (also 3 => 'Three').
I am working on a complete solution right now.

As I suspected, "(string)" cast in the in_array() is necessary here to avoid backfiring.
All tests pass now.

Also: When refactoring the form helper and "required/disabled" we might only have taken into account "multiple checkboxes".
But "multiple select" does not work. This fixes it.
Also added tests.

Owner

markstory commented Feb 1, 2013

Does the disabled attribute on <option> elements work now? I remember a time when many browsers ignored that request.

@markstory markstory commented on the diff Feb 1, 2013

lib/Cake/Test/Case/View/Helper/FormHelperTest.php
+ ),
+ array('option' => array('value' => '1')),
+ 'One',
+ '/option',
+ array('option' => array('value' => '2', 'selected' => 'selected')),
+ 'Two',
+ '/option',
+ array('option' => array('value' => '3')),
+ 'Three',
+ '/option',
+ array('option' => array('value' => '3x', 'selected' => 'selected')),
+ 'Stringy',
+ '/option',
+ '/select'
+ );
+ $this->assertTags($result, $expected);
@markstory

markstory Feb 1, 2013

Owner

What is this test covering? It doesn't seem to be related to the change at hand.

@dereuromark

dereuromark Feb 1, 2013

Member

If I am not mistaken this is the most important one. Proofing that the (string) cast is necessary. Otherwise the previous fixes would still be left incomplete.
Note my example.

Owner

markstory commented Feb 1, 2013

The change looks good to me.

Member

dereuromark commented Feb 1, 2013

I tested it in chrome and FF - worked just fine for me...

We should make sure that the 2.2 also gets that back-ported. Especially the part about about the in_array() fix.

Owner

markstory commented Feb 1, 2013

You mean forward ported. I'm fairly certain @rchavik already merged the temporary 2.2 branch into master.

Member

dereuromark commented Feb 1, 2013

This is an additional fix to the previous ones he and I merged in. So I am fairly certain that the test cases here would not pass in 2.2 at the moment. Especially without the missing (string) casts in the in_array() call.

@dereuromark dereuromark commented on the diff Feb 6, 2013

lib/Cake/Test/Case/View/Helper/FormHelperTest.php
@@ -3663,7 +3663,7 @@ public function testRadioDisabled() {
$result = $this->Form->radio(
'Model.field',
array('option A', 'option B'),
- array('disabled' => array('option A'), 'value' => '0')
+ array('disabled' => array(0), 'value' => '0')
@dereuromark

dereuromark Feb 6, 2013

Member

Was the orignal "optionA" a valid input here? I would expect only keys to be valid values here...

@markstory

markstory Feb 6, 2013

Owner

I don't think it was valid.

@dereuromark

dereuromark Feb 6, 2013

Member

I thought so. Another one of those bugs due to this stupid in_array() behavior.

So the current tests are fine then? Does everything look OK to be merged?

Member

rchavik commented Feb 7, 2013

looks okay to me.. 👍

@markstory markstory commented on an outdated diff Feb 7, 2013

lib/Cake/Test/Case/View/Helper/FormHelperTest.php
@@ -4256,6 +4303,95 @@ public function testSelectMultiple() {
'/select'
);
$this->assertTags($result, $expected);
+
+ $options = array(1 => 'One', 2 => 'Two', '3' => 'Three', '3x' => 'Stringy');
+ $selected = array('2', '3x');
+ $result = $this->Form->select(
+ 'Model.multi_field', $options, array('multiple' => false, 'value' => $selected)
@markstory

markstory Feb 7, 2013

Owner

Why is this multiple false? Selecting two elements in a single select box is odd.

Owner

markstory commented Feb 7, 2013

Outside of the one remaining question I had, this looks good to me as well.

@dereuromark dereuromark Fix disabled elements as array for multiple select and make in_array(…
…) work properly here, fix same in_array issues for radio elements and move tests correctly - #1105
d522f41

@dereuromark dereuromark added a commit that referenced this pull request Feb 7, 2013

@dereuromark dereuromark Merge pull request #1105 from dereuromark/master-fix-disabled
Resolve incomplete regression fix for FormHelper selection and disabled.
abf1bd8

@dereuromark dereuromark merged commit abf1bd8 into cakephp:master Feb 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment