Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
finish #13598#18047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
finish #13598#18047
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedMar 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.'.
xabbuh commentedMar 7, 2016
HeahDude commentedMar 7, 2016
The tests are still skipped, even with#18019 :( |
xabbuh commentedMar 7, 2016
At least, on AppVeyor they are executed and pass. |
HeahDude commentedMar 7, 2016
Great! |
xabbuh commentedMar 7, 2016
3006020 toaa5e45bCompare| publicfunctiontestSubmitNullWithEmptyDataTrueWhenNested() | ||
| { | ||
| $form =$this->factory->create('form') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Neither this one nortestSubmitNullWithEmptyData(which is the same without nesting) do pass without the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh okay, got it now what you were talking about. I changed theCheckboxType tests to not use nested forms and removed the nested test from theIntegerTypeTest where it also wasn't necessary.
HeahDude commentedMar 7, 2016
Also, I confirm I still cannot close#17822 in favor of this one. It remains a singular case. |
HeahDude commentedMar 7, 2016
👍 Status: Reviewed |
| publicfunctiontestSubmitNullWithEmptyDataTrue() | ||
| { | ||
| $form =$this->factory->create('checkbox',null,array( | ||
| 'empty_data' =>true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. Theempty_data valuetrue 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 theempty_data incorrectly, by passing data in model instead of view format (ping@guilhermeblanco).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But it's valid with default settings, right ?
In that test$this->assertSame('1', $viewData) would pass.
guilhermeblanco commentedMar 8, 2016
@xabbuh@webmozart my case was explicitly with integer types, not checkboxes (which are boolean values). |
HeahDude commentedMar 8, 2016
@guilhermeblanco Fortunately your fix solves both :) Thanks. |
webmozart commentedMar 9, 2016
@guilhermeblanco Did you pass integers or strings (integer strings) to |
webmozart commentedMar 9, 2016
xabbuh commentedMar 10, 2016
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). |
HeahDude commentedMar 10, 2016
Agreed, I guess we all learned something with this one. @xabbuh I could deal with insymfony/symfony-docs#6265, what do you think ? |
xabbuh commentedMar 10, 2016
guilhermeblanco commentedMar 10, 2016
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. |
xabbuh commentedMar 11, 2016
@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. |
webmozart commentedMar 14, 2016
@guilhermeblanco I agree with you. Unfortunately we can't change that for the moment due to BC implications. |