Skip to content
This repository has been archived by the owner on Feb 26, 2018. It is now read-only.

Fix checkbox array binding #78

Merged
merged 10 commits into from
Feb 19, 2016
Merged

Fix checkbox array binding #78

merged 10 commits into from
Feb 19, 2016

Conversation

jesseleite
Copy link
Contributor

Old input and model binding worked fine multiple selects (this PR adds test coverage to prove this):

$form->select('favourite_foods[]')
     ->options(['fish' => 'Fish', 'tofu' => 'Tofu', 'chips' => 'Chips'])
     ->multiple()

But doesn't work on checkbox arrays:

$form->checkbox('favourite_foods[]', 'fish')
$form->checkbox('favourite_foods[]', 'tofu')
$form->checkbox('favourite_foods[]', 'chips')

This PR adds test coverage and a proposed fix for checkbox array old input and model binding.

@jesseleite
Copy link
Contributor Author

Ready for review 🎱

@@ -76,11 +76,14 @@ protected function setChecked($checked = true)

protected function checkBinding()
{
$currentValue = $this->getAttribute('value');
$currentValue = (string) $this->getAttribute('value');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain all the string casting stuff in more detail and what the issue was that made you need to do it? It would be nice if it's avoidable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure man. So at first === made sense to me, but then this happened. I was minding my own business trying to implement simple radios with binary values...

$form->radio('published', 1)->render();
$form->radio('published', 0)->render();

In context of Laravel/Eloquent, these values are not cast to integers unless I setup an accessor on my model and intval() the thing. However, the value stored on the field object is stored as integer. So by default the old input provider shows the user's selected value as string, but the value on the element object is integer, causing this 1 === "1" to not bind the old input.

The problem is, we can't simply 1 == "1". Imagine this scenario...

<input type="radio" name="published" value="0">

If there is no old value ($oldValue on the field object is then null), then doing a 0 == null equates to true, and the radio is checked, as if it were old input! So in this case we DO WANT ===, but in the first case we don't want ===.

Casting everything to string before comparing values solves both problems. TL;DR... The HTML value attribute on checkboxes/radios knows nothing about an integer, or anything about data types or null values for that matter. All it knows is string value. If we cast everything to string first, we get a more accurate comparison of old input in all cases.

adamwathan added a commit that referenced this pull request Feb 19, 2016
@adamwathan adamwathan merged commit a8fc8b9 into adamwathan:master Feb 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants