Skip to content

Commit

Permalink
ENHANCEMENT Support numeric array values in CheckboxSetField (?Field[…
Browse files Browse the repository at this point in the history 
…]=val1&Field[]=val2 instead of ?Field[val1]=1&Field[val2]=1)
  • Loading branch information
@chillu
chillu committed Mar 2, 2012
1 parent 98e1fed commit 8fa2664
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 17 deletions.
15 changes: 10 additions & 5 deletions forms/CheckboxSetField.php
View file
Expand Up @@ -184,10 +184,16 @@ function saveInto(DataObject $record) {
$fieldname = $this->name ;
if($fieldname && $record && ($record->has_many($fieldname) || $record->many_many($fieldname))) {
$idList = array();
if($this->value) foreach($this->value as $id => $bool) {
if($bool) {
$idList[] = $id;
}
// Works for both <select multiple> style - array(0 => 'val1', 1 => 'val2')
// and <input type="checkbox"> style - array('val1' => true, 'val2' => true).
// The <select multiple> element doesn't allow for individual keys in parameter names.
$valuesInKeys = (ArrayLib::is_associative($this->value));

This comment has been minimized.

Sign in to view

Copy link
@sminnee

sminnee Mar 5, 2012

Member

This looks dodgy. What if the data just happens to meet the ArrayLib::is_associative criteria but isn't supposed to be used that way? Perhaps you should do check the field name instead - if it ends with [] then you know that the values are supposed to be in the

Of course, this need to deal with two completely different data formats is a result of the fact that you want to butcher CheckboxSetField to turn it into a MultiItemSelectField. Perhaps it would be better to separate these two field types explicitly? Yes, the data is similar but there's a lot about the two fields that are different.

The dev cycle appears to have been "oh, hey, it's just a template update! oh, wait, there's also a data format change." I'm not sure what the value in keeping these two fields within a single class is, but I'd suggest you reconsider.

This comment has been minimized.

Sign in to view

Copy link
@chillu

chillu Mar 5, 2012

Author Member

The dev cycle was "Copy code to ListboxField to make it work properly with relations, find out its a pain in the ass, try to cram it into CheckboxSetField" ;)
I figured it was too much of a distraction to fix up ListboxField for the goal at hand, but done that now in 6e3ceef, and reverted this commit. Also, I assume your envisioned MultiItemSelectField matches the existing ListboxField?
if($this->value) foreach($this->value as $k => $v) {
if($valuesInKeys) {
if($v) $idList[] = $k;
} else {
$idList[] = $v;
}
}
$record->$fieldname()->setByIDList($idList);
} elseif($fieldname && $record) {
Expand All @@ -214,7 +220,6 @@ function dataValue() {
$filtered[] = str_replace(",", "{comma}", $item);
}
}

return implode(',', $filtered);
}

Expand Down
36 changes: 35 additions & 1 deletion tests/forms/CheckboxSetFieldTest.php
View file
Expand Up @@ -65,7 +65,7 @@ function testSaveWithNothingSelected() {
);
}

function testSaveWithArrayValueSet() {
function testSaveWithAssociativeArrayValueSet() {
$article = $this->objFromFixture('CheckboxSetFieldTest_Article', 'articlewithouttags');
$articleWithTags = $this->objFromFixture('CheckboxSetFieldTest_Article', 'articlewithtags');
$tag1 = $this->objFromFixture('CheckboxSetFieldTest_Tag', 'tag1');
Expand Down Expand Up @@ -98,6 +98,40 @@ function testSaveWithArrayValueSet() {
'Data shold be saved into CheckboxSetField manymany relation table on the "left end"'
);
}

function testSaveWithNumericArrayValueSet() {
$article = $this->objFromFixture('CheckboxSetFieldTest_Article', 'articlewithouttags');
$articleWithTags = $this->objFromFixture('CheckboxSetFieldTest_Article', 'articlewithtags');
$tag1 = $this->objFromFixture('CheckboxSetFieldTest_Tag', 'tag1');
$tag2 = $this->objFromFixture('CheckboxSetFieldTest_Tag', 'tag2');

/* Create a CheckboxSetField with 2 items selected. Note that the array is in the format (key) => (selected) */
$field = new CheckboxSetField("Tags", "Test field", DataObject::get("CheckboxSetFieldTest_Tag")->map());
$field->setValue(array(
$tag1->ID,
$tag2->ID
));

/* Saving should work */
$field->saveInto($article);

$this->assertEquals(
array($tag1->ID,$tag2->ID),
DB::query("SELECT \"CheckboxSetFieldTest_TagID\"
FROM \"CheckboxSetFieldTest_Article_Tags\"
WHERE \"CheckboxSetFieldTest_Article_Tags\".\"CheckboxSetFieldTest_ArticleID\" = $article->ID
")->column(),
'Data shold be saved into CheckboxSetField manymany relation table on the "right end"'
);
$this->assertEquals(
array($articleWithTags->ID,$article->ID),
DB::query("SELECT \"CheckboxSetFieldTest_ArticleID\"
FROM \"CheckboxSetFieldTest_Article_Tags\"
WHERE \"CheckboxSetFieldTest_Article_Tags\".\"CheckboxSetFieldTest_TagID\" = $tag1->ID
")->column(),
'Data shold be saved into CheckboxSetField manymany relation table on the "left end"'
);
}

function testLoadDataFromObject() {
$article = $this->objFromFixture('CheckboxSetFieldTest_Article', 'articlewithouttags');
Expand Down
13 changes: 2 additions & 11 deletions tests/forms/CheckboxSetFieldTest.yml
View file
Expand Up @@ -3,17 +3,8 @@ CheckboxSetFieldTest_Tag:
Title: Tag 1
tag2:
Title: Tag 2
CheckboxSetFieldTest_Article:
articlewithouttags:
Content: Article 1
articlewithtags:
Content: Article 2
Tags: =>CheckboxSetFieldTest_Tag.tag1,=>CheckboxSetFieldTest_Tag.tag2
CheckboxSetFieldTest_Tag:
tag1:
Title: Tag 1
tag2:
Title: Tag 2
tag3:
Title: Tag 3
CheckboxSetFieldTest_Article:
articlewithouttags:
Content: Article 1
Expand Down

0 comments on commit 8fa2664

Please sign in to comment.