[Form] [ChoiceType] Prefer placeholder to empty_value #16886

boite · 2015-12-07T15:23:15Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#16885
License	MIT
Doc PR	-

Prefer an explicitly set placeholder option (i.e. false or a non-empty
string) to an empty_value option when both are set.

The fix is to change the behaviour in the placeholder normalizer in
ChoiceType::configureOptions so that the value of the empty_value option is
used for placeholder only when the value of placeholder is null or an empty
string.

Use assertSame instead of assertEquals in some ChoiceTypeTest methods which test placeholder/empty_value options, because the tested value may be any number of "falsey" values.

Add an extra condition to the use of the value of the empty_value option as the placeholder: the value of the placeholder option must be null or an empty string (i.e. not explicitly set). Test the effects of setting both placeholder and empty_value.

HeahDude · 2016-02-14T22:36:32Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php

+            'An unset empty_value is automaticaly made an empty string in a non-required field (but null is expected here) [maintains BC]' => array(false, false, false, null, null, ''),
+            'An empty string empty_value is used if placeholder is not set [maintains BC]' => array(false, false, false, null, '', ''),
+            'A non-empty string empty_value is used if placeholder is not set [maintains BC]' => array(false, false, false, null, 'bar', 'bar'),
+            'A placeholder is not used if it is an empty string and empty_value is set to false [mainatins BC]' => array(false, false, false, '', false, null),


typo "maintains"

HeahDude · 2016-02-14T22:40:50Z

👍 except your last test being very difficult to read.

Perhaps you could organise lines by BC first, then by expanded or multiple.
Rephrasing might help like :
"If placeholder is null", "If placeholder is false and empty_value is empty" ...

xabbuh · 2016-02-18T11:33:04Z

The fix looks good, but do we really need so many test fixtures? Can we reduce them to the bare minimum?

boite · 2016-02-28T12:48:26Z

@xabbuh the (admittedly extensive) test data were invaluable in that they revealed when the fix was finally correct after a number of failed attempts. Surely, one datum for each possible permutation is the bare minimum.

fabpot · 2016-02-29T17:07:05Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php

+        $placeholder,
+        $emptyValue,
+        $viewValue
+    ) {


should be moved on one line

fabpot · 2016-02-29T17:07:23Z

👍

webmozart · 2016-03-02T09:16:29Z

👍

fabpot · 2016-03-02T09:55:02Z

Thank you @boite.

This PR was squashed before being merged into the 2.7 branch (closes #16886). Discussion ---------- [Form] [ChoiceType] Prefer placeholder to empty_value | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16885 | License | MIT | Doc PR | - Prefer an explicitly set `placeholder` option (i.e. `false` or a non-empty string) to an `empty_value` option when both are set. The fix is to change the behaviour in the placeholder normalizer in ChoiceType::configureOptions so that the value of the `empty_value` option is used for placeholder only when the value of `placeholder` is null or an empty string. Commits ------- a4d4c8a [Form] [ChoiceType] Prefer placeholder to empty_value

HeahDude · 2016-03-02T10:12:44Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php

+            'A placeholder is not used if it is explicitly set to false' => array(false, false, false, false, null, null),
+            'A placeholder is not used if it is explicitly set to false' => array(false, false, false, false, '', null),
+            'A placeholder is not used if it is explicitly set to false' => array(false, false, false, false, 'bar', null),
+            'A placeholder is not used if empty_value is set to false [maintains BC]' => array(false, false, false, null, false, null),


All those tests may not run because some keys are overridden...

HeahDude · 2016-03-02T10:39:14Z

Here's the actual report :

There were 8 failures:

1) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "An empty string empty_value is converted to "None" in an expanded single choice field when expanded and required [maintains BC]" (false, true, true, null, '', 'None')
Failed asserting that null is identical to 'None'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

2) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "A non-empty string empty_value is used if placeholder is not set when expanded and required [maintains BC]" (false, true, true, null, 'bar', 'bar')
Failed asserting that null is identical to 'bar'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

3) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "An empty string empty_value is converted to "None" in an expanded single choice field when required [maintains BC]" (false, true, true, '', '', 'None')
Failed asserting that null is identical to 'None'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

4) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "A non-empty string empty_value is used if placeholder is an empty string when expanded and required [maintains BC]" (false, true, true, '', 'bar', 'bar')
Failed asserting that null is identical to 'bar'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

5) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "A non-empty string placeholder takes precedence over an empty_value set to false when expanded and required" (false, true, true, 'foo', false, 'foo')
Failed asserting that null is identical to 'foo'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

6) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "A non-empty string placeholder takes precedence over a not set empty_value when expanded and required" (false, true, true, 'foo', null, 'foo')
Failed asserting that null is identical to 'foo'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

7) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "A non-empty string placeholder takes precedence over an empty string empty_value when expanded and required" (false, true, true, 'foo', '', 'foo')
Failed asserting that null is identical to 'foo'.

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1913

8) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testPlaceholderOptionWithEmptyValueOption with data set "A non-empty string placeholder takes precedence over a non-empty string empty_value when expanded and required" (false, true, true, 'foo', 'bar', 'foo')
Failed asserting that null is identical to 'foo'.

boite · 2016-03-02T10:53:26Z

Damn it! Will you let me sort this out? I'll do a better job of naming the data keys for readability (and uniqueness).

HeahDude · 2016-03-02T11:32:51Z

I fixed it, but I still can hold the PR. What do I do ?

HeahDude · 2016-03-02T17:10:39Z

@boite, see #17988, feel free to send a PR that better suits you and I'll close this one.

This PR was merged into the 2.7 branch. Discussion ---------- [Form] fix tests added by #16886 | Q | A | ------------- | --- | Branch | 2.7+ | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16886 | License | MIT | Doc PR | - Commits ------- bd22c86 minor [Form] fix tests added by #16886

* 2.7: Updated all the README files [TwigBundle] Fix failing test on appveyor Improved the error message when using "@" in a decorated service Improve error reporting in router panel of web profiler [DoctrineBridge][Form] Fix performance regression in EntityType [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths Allow to normalize \Traversable minor [Form] fix tests added by #16886 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener Simplified everything Added a test Fixed the problem in an easier way Fixed a syntax issue Improved the error message when a template is not found [CodingStandards] Conformed to coding standards [TwigBundle] fixed Include file locations in "Template could not be found" exception

* 2.8: Updated all the README files [TwigBundle] Fix failing test on appveyor Improved the error message when using "@" in a decorated service Improve error reporting in router panel of web profiler [DoctrineBridge][Form] Fix performance regression in EntityType [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths Allow to normalize \Traversable minor [Form] fix tests added by #16886 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener Simplified everything Added a test Fixed the problem in an easier way Fixed a syntax issue Improved the error message when a template is not found [CodingStandards] Conformed to coding standards [TwigBundle] fixed Include file locations in "Template could not be found" exception

* 3.0: Updated all the README files [TwigBundle] Fix failing test on appveyor Improved the error message when using "@" in a decorated service Improve error reporting in router panel of web profiler [DoctrineBridge][Form] Fix performance regression in EntityType [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths Allow to normalize \Traversable minor [Form] fix tests added by #16886 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener Simplified everything Added a test Fixed the problem in an easier way Fixed a syntax issue Improved the error message when a template is not found [CodingStandards] Conformed to coding standards [TwigBundle] fixed Include file locations in "Template could not be found" exception

* 2.7: Updated all the README files [TwigBundle] Fix failing test on appveyor Improved the error message when using "@" in a decorated service Improve error reporting in router panel of web profiler [DoctrineBridge][Form] Fix performance regression in EntityType [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths Allow to normalize \Traversable minor [Form] fix tests added by symfony#16886 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener Simplified everything Added a test Fixed the problem in an easier way Fixed a syntax issue Improved the error message when a template is not found [CodingStandards] Conformed to coding standards [TwigBundle] fixed Include file locations in "Template could not be found" exception

* 2.8: Updated all the README files [TwigBundle] Fix failing test on appveyor Improved the error message when using "@" in a decorated service Improve error reporting in router panel of web profiler [DoctrineBridge][Form] Fix performance regression in EntityType [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths Allow to normalize \Traversable minor [Form] fix tests added by symfony#16886 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener Simplified everything Added a test Fixed the problem in an easier way Fixed a syntax issue Improved the error message when a template is not found [CodingStandards] Conformed to coding standards [TwigBundle] fixed Include file locations in "Template could not be found" exception

* 3.0: Updated all the README files [TwigBundle] Fix failing test on appveyor Improved the error message when using "@" in a decorated service Improve error reporting in router panel of web profiler [DoctrineBridge][Form] Fix performance regression in EntityType [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths Allow to normalize \Traversable minor [Form] fix tests added by symfony#16886 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener Simplified everything Added a test Fixed the problem in an easier way Fixed a syntax issue Improved the error message when a template is not found [CodingStandards] Conformed to coding standards [TwigBundle] fixed Include file locations in "Template could not be found" exception

carsonbot added the Status: Needs Review label Dec 7, 2015

boite force-pushed the ticket_16885 branch 4 times, most recently from 4271ab1 to 4dec2c8 Compare December 14, 2015 10:24

boite added 2 commits December 27, 2015 13:12

[Form] Use strict comparison assertions in tests

3410779

Use assertSame instead of assertEquals in some ChoiceTypeTest methods which test placeholder/empty_value options, because the tested value may be any number of "falsey" values.

boite force-pushed the ticket_16885 branch from 4dec2c8 to 9ed36d1 Compare December 27, 2015 13:12

HeahDude reviewed Feb 14, 2016
View reviewed changes

Fix typos in test dataset names.

3c31a91

fabpot reviewed Feb 29, 2016
View reviewed changes

Collapse multi-line test method sig.

a5d4e3f

fabpot closed this Mar 2, 2016

HeahDude reviewed Mar 2, 2016
View reviewed changes

HeahDude added a commit to HeahDude/symfony that referenced this pull request Mar 2, 2016

minor [Form] fix tests added by symfony#16886

bd22c86

HeahDude mentioned this pull request Mar 2, 2016

[Form] fix tests added by #16886 #17988

Merged

This was referenced Mar 25, 2016

Release v2.7.11 #18315

Merged

Release v2.8.4 #18327

Closed

Release v2.8.4 #18329

Merged

Release v3.0.4 #18363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] [ChoiceType] Prefer placeholder to empty_value #16886

[Form] [ChoiceType] Prefer placeholder to empty_value #16886

boite commented Dec 7, 2015

HeahDude Feb 14, 2016

HeahDude commented Feb 14, 2016

xabbuh commented Feb 18, 2016

boite commented Feb 28, 2016

fabpot Feb 29, 2016

fabpot commented Feb 29, 2016

webmozart commented Mar 2, 2016

fabpot commented Mar 2, 2016

HeahDude Mar 2, 2016

fabpot Mar 2, 2016

HeahDude Mar 2, 2016

HeahDude commented Mar 2, 2016

boite commented Mar 2, 2016

HeahDude commented Mar 2, 2016

HeahDude commented Mar 2, 2016

[Form] [ChoiceType] Prefer placeholder to empty_value #16886

[Form] [ChoiceType] Prefer placeholder to empty_value #16886

Conversation

boite commented Dec 7, 2015

HeahDude Feb 14, 2016

Choose a reason for hiding this comment

HeahDude commented Feb 14, 2016

xabbuh commented Feb 18, 2016

boite commented Feb 28, 2016

fabpot Feb 29, 2016

Choose a reason for hiding this comment

fabpot commented Feb 29, 2016

webmozart commented Mar 2, 2016

fabpot commented Mar 2, 2016

HeahDude Mar 2, 2016

Choose a reason for hiding this comment

fabpot Mar 2, 2016

Choose a reason for hiding this comment

HeahDude Mar 2, 2016

Choose a reason for hiding this comment

HeahDude commented Mar 2, 2016

boite commented Mar 2, 2016

HeahDude commented Mar 2, 2016

HeahDude commented Mar 2, 2016