finish #13598 #18047
finish #13598 #18047
Conversation
xabbuh
commented
Mar 7, 2016
| Q | A |
|---|---|
| Branch | 2.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #13598 |
| License | MIT |
| Doc PR |
… it throws a TransformationFailedException with message: 'Expected a string.'.
|
The tests are still skipped, even with #18019 :( |
|
At least, on AppVeyor they are executed and pass. |
|
Great! |
3006020 to
aa5e45b
Compare
|
|
||
| public function testSubmitNullWithEmptyDataTrueWhenNested() | ||
| { | ||
| $form = $this->factory->create('form') |
There was a problem hiding this comment.
Do the tests pass (before the change) if this is not nested? If not, I'd simplify this test.
There was a problem hiding this comment.
Neither this one nor testSubmitNullWithEmptyData(which is the same without nesting) do pass without the change.
There was a problem hiding this comment.
Ok, actually @webmozart is right.
You can remove "WhenNested" is the method name and refractor this to:
$form = $this->factory->create('checkbox', null, array(
'empty_data' => true,
));
$form->submit(null);
$this->assertTrue($form->getData());
$this->assertSame('1', $form->getViewData());There was a problem hiding this comment.
Oh okay, got it now what you were talking about. I changed the CheckboxType tests to not use nested forms and removed the nested test from the IntegerTypeTest where it also wasn't necessary.
|
Also, I confirm I still cannot close #17822 in favor of this one. It remains a singular case. |
|
👍 Status: Reviewed |
| public function testSubmitNullWithEmptyDataTrue() | ||
| { | ||
| $form = $this->factory->create('checkbox', null, array( | ||
| 'empty_data' => true, |
There was a problem hiding this comment.
Actually, this code is invalid. empty_data expects data in view format, not in model format. The view format depends on the transformer that is used.
Imagine the field is configured with the following transformer
true <---> 'yes'
false <---> 'no'Then this code will fail. The empty_data value true will be cast to the string '1'. Consequently, the transformer fails, since it doesn't know how to reverse transform '1'.
The correct test would be:
$form = $this->factory->create('checkbox', null, array(
'empty_data' => '1',
));I understand this is not very straight-forward nor intuitive, but we can't change that for the moment.
I believe that the use case that motivated this change was actually using the empty_data incorrectly, by passing data in model instead of view format (ping @guilhermeblanco).
There was a problem hiding this comment.
But it's valid with default settings, right ?
In that test $this->assertSame('1', $viewData) would pass.
|
@xabbuh @webmozart my case was explicitly with integer types, not checkboxes (which are boolean values). |
|
@guilhermeblanco Fortunately your fix solves both :) Thanks. |
|
@guilhermeblanco Did you pass integers or strings (integer strings) to |
|
I guess we can close here. Changing the test to use the string Thanks for raising the issue on the docs repo @webmozart. This is indeed something that we have to improve (I didn't know that before). |
|
Agreed, I guess we all learned something with this one. @xabbuh I could deal with in symfony/symfony-docs#6265, what do you think ? |
|
OMG... Using string as workaround was detailed in the original ticket that it worked. However, that's not a solution. If you declare an integer field, you expect an integer back, not a string. |
|
@guilhermeblanco Sorry for the inconvenience. Though it turns out that using a string is not a workaround but the right solution as this is how the view data for the integer type look like. |
|
@guilhermeblanco I agree with you. Unfortunately we can't change that for the moment due to BC implications. |