Fix CheckboxSetField's getOptions() $items creation

[pief]

This change fixes a bug in framework/forms/CheckboxSetField.php's
getOptions() method that triggered warnings such as the following:

Warning at framework/forms/CheckboxSetField.php line 167: in_array()
expects parameter 2 to be array, object given (http://foo/admin/
pages/edit/EditForm/field/Dishes/item/416/edit)

This was due to $items being expected to always be an array, which didn't
hold true for the combination of $source being an Array and $values being
an instanceof SS_List object.

[pief]

This is the patch from #3777, this time as a pull request for easier merging.

[kinglozzer]

Thanks @pief, the patch seems to make sense. It’d be good to get some test coverage for this: ideally a test for each of the different possible source/value combinations.

[dhensby]

Yes, also a test that would have failed before this patch. Thanks.

[pief]

I have no knowledge about writing SilverStripe test cases yet, but let me see if I can find an example.

[dhensby]

@pief there are a ton of tests in the framework you can use as a starting point.

Start by looking at CheckboxSetFieldTest.php and adding a test function to that class that basically instantiates a CheckboxSetField that would have caused the unexpected behaviour you're solving here.

[dhensby]

Hmm, the docs do explicitly state that if the options are an array then the values should be too. so this isn't a bug and is (in fact) intended functionality.

ASSUMPTION -> IF you pass your source as an array, you pass values as an array too. Likewise objects are handled the same.

Therefore, if this behaviour is to change in a BC way it needs to be against 3 branch

[pief]

Well, in that case the docs (I assume you mean the comment at the top of CheckboxSetField.php) do not match the current code or vice versa, seeing that the current code explicitly tries to handle mixed cases.

[pief]

I've been spending some more time tracking down how I actually hit this behavior. I'm still convinced that my patch is right, seeing that it implements the same behaviour as the current code, only in a simpler fashion. But your comment about the "documented behavior" made me curious if my own code is the culprit.

So I came up with the following example:

• DailyMenuPage a ''Page'' that contains a GridView for
• Dish DataObjects with a ''Many_Many'' relationship with
• DishLabel

Now I can trigger the behavior if in Dish I rename DishLabels() to getDishLabels(). Then, Form's loadDataFrom() will get an Object of type ManyManyList which will be stored in the CheckboxSetField's $value property, causing the described situation in getOptions() later on.

[pief]

Also, if I implement getDishlabels() like this:

    public function getDishLabels() {
        return $this->DishLabels()->map()->toArray();
    }

while I get no more error the checkboxes on the edit form do not get checked anymore. So apparantly it is expected to return a ManyManyList (that gets stored in CheckboxSetField's $value and thus picked up by getOptions()).

[stevie-mayhew]

@pief - is this a contrived example or something which you've actually needed to do? I'm wondering if we are overcomplicating what will eventually become CheckboxSetField input validation rather than being in the getOptions method?

[dhensby]

is this a contrived example or something which you've actually needed to do

Yes, I'm struggling to completely understand this and why it's a problem and how it's not working as expected.

Tests are somewhat essential to allowing us to understand and to demonstrate it's a real world problem.

[dhensby]

If tests are a struggle, simply create a Gist with the code that causes an issue and I'll look at turning it into a test for you.

[pief]

Well, naturally this was meant to be an easier, standalone example. The actual code I have at hand here is, of course, more complex: there is a Dish class and also a CustomDish class, but CustomDish is meant to exist within the database only. Dish has magic methods that regard the current subsite and do the linking to the appropriate CustomDish, which holds additional subsite-specific data -- in this case, the DishLabels.

So in the real world Dish's getDishLabels() actually determines the correct CustomDish and then calls out to CustomDish->DishLabels() and returns it.

[pief]

So, CustomDish has the Many-Many-Relationship on DishLabel, CustomDish->DishLabels() returns a ManyManyList object and thus Dish->getDishLabels() does so as well.

[dhensby]

@pief if you can devise the least complex example where this is a problem ,then that would be good

[pief]

For a test case? Yes, I can probably come up with something, but so the use case is clear now? Basically any "virtual property" method getFoo() that calls out some other class' Many-Many-Relationship-imposed method Foo()?

[dhensby]

@pief this needs rebasing and still requires some tests

[tractorcow]

This has probably been fixed in master, see #4568.

Still worth fixing in 3.x