Fix bug: DropdownField::create(...)->setHasEmptyDefault(true) was not effective

[nathanbrauer]

Found this bug when upgrading to 3.1.

Our original code in Page.php (which should work on any standard CMS setup):

$fields->addFieldTotab('Root.Metadata',DropdownField::create("AuthorID",'Author', ArrayList::create(DataList::create('Member'))->map('ID','AuthorName'))->setHasEmptyDefault(true));

Was updated to:

$fields->addFieldTotab('Root.Metadata',DropdownField::create("AuthorID",'Author', DataList::create('Member')->map('ID','AuthorName'))->setHasEmptyDefault(true));

The only difference is using ArrayList instead of DataList (working around a bug in 3.0).

I tracked down the bug to DropdownField.php where it was checking $this->emptyString instead of $this->getHasEmptyDefault().

[nathanbrauer]

There's already a test for this, but maybe it's not done properly?
https://github.com/silverstripe/silverstripe-framework/blob/master/tests/forms/DropdownFieldTest.php#L41

I'm not familiar enough to know one way or the other.

[nathanbrauer]

Last two commits properly cleans up the code and adds more in-depth tests.

[wilr]

This fails tests related to DropdownField https://travis-ci.org/silverstripe/silverstripe-framework/builds/12347466

[nathanbrauer]

@wilr - I'm not quite sure how to read that page. Unit testing is new to me. :)
Could you or @chillu take a look for me and let me know what I need to do?

[wilr]

As the comment for getSource() says "Gets the source array including any empty default values." you're changing that behaviour so we'll need to put this into 3.2 rather to 3.1 as a start as it's not crazy for another developer to use getSource() on another drop down.

It looks like the tests assume the behaviour that getSource() will return the default values. For me, it makes better sense for getSource() to not return the blank value so I agree with the PR, you'll just need to submit against 3.2 and play around with DropdownFieldTest. Any questions on testing I'm happy to help.

[nathanbrauer]

3.2 makes sense.

I'm not sure if you noticed, but I updated the test. I'm not sure if the unit test validated using the new one (d1808c3) or using the old one (1d7a05c or before).

https://github.com/nathanbrauer/silverstripe-framework/blob/d1808c35afba7f4a641fc8ec91bce6b4b26e2f4b/tests/forms/DropdownFieldTest.php#L41

Before my changes to the test:

1. If setHasEmpty(true)
   1. validates DropdownField()->getSource() (expects empty fields)

It now checks:

1. If setHasEmpty(true)
   1. validates DropdownField()->getSource() (expects _no_ empty fields)
   2. validates DropdownField()->Field()->getValue() (_expects_ empty fields)
2. If setHasEmpty(false)
   1. validates DropdownField()->getSource() (expects _no_ empty fields)
   2. validates DropdownField()->Field()->getValue() (expects _no_ empty fields)

[chillu]

@wilr Could you peer review please? :)

[wilr]

Ok so the tests still needed a bit of work, most of the test cases you added were already present and needed to be updated for said API change to getSource(). Tidied and pushed to master in fee54c7 thanks