Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Form] LetTextType implementDataTransformerInterface#18357
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
[Form] LetTextType implementDataTransformerInterface#18357
Uh oh!
There was an error while loading.Please reload this page.
Conversation
HeahDude commentedMar 30, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #5906 |
| License | MIT |
| Doc PR | ~ |
4ad7303 tof274178CompareFailures unrelated. |
This goes in master, right ? |
TextType implementsDataTransformerInterfaceTextType implementDataTransformerInterface| $this->assertSame('', $form->getData()); | ||
| } | ||
| } |
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.
Please add a newline at the end of this file. Thanks!
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.
@javiereguiluz Nice catch! Thanks :)
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.
Fixed.
f274178 to2a66b4eCompare| */ | ||
| public function reverseTransform($data) | ||
| { | ||
| return empty($data) ? '' : $data; |
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.
Should benull === $data ? '' : $data, no ?
What happens if we submit0 ?
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.
@fsevestre Thanks! I've changed it will making the tests and forgot to revert it :)
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.
Fixed!
closessymfony#5906.The submitted data should always betransformed back to the model as a stringas NULL in this case could stand for "unsetthis value" whereas a string property of aclass could rely on the string type.Furthermore, this prevents potential issueswith PHP 7 which allows type hinting ofstrings in functions.
2a66b4e to7e97fbbCompare| 'empty_data' => '', | ||
| )); | ||
| $form->submit(null); |
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.
is there any config with the new system that allows one to distinguish betweensubmit(null) andsubmit('') in the model data?
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.
@backbone87 I don't really understand what is "new" for you, as I am feeling quite new myself in symfony :)
But as far as I understand when you call
Form::submit(null)here what happens:- view data is initialized with null,
- if not
compoundand notinherit_data(e.gTextType)view data is set to submitted data whichmay benulland would not be converted to empty string
So at this point view data cannot be an empty string, it remains
null.Then:
- view data is assigned to
empty_data, either simply set or acallable getting the form andnull - the final resolution as before anything we must be sure thatif compound data must be an array, if not it must be a string.
- view data is assigned to
Hence here we do have a view data as empty string but...:
- here comes the devil because before getting to the model datathe normalization set it to null :/,
Form::viewToNorm()is actually the default view data transformer,
Conclusion
This PR could add a view data transformer toprevent that call but I guess in order to prevent messing with other transformers and as this has to stay the default,@webmozart asked to add it as a model data transformer since it is the first registered => also the last called => minimum of potential BC breaks.
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.
with "new system" i was refering to the state of the form component after your PR.
i was specially talking about how to distinguish between a text field that is submitted with null vs submitted with empty string.
consider a simple form with text field:
$form =$factory->create();$form->add('name', TextType::class, ['required' =>false ]);
and now the different submits:
// 1.$form->submit(null);assert$form->get('name')->getData() ===null;// 2.$form->submit([]);assert$form->get('name')->getData() ===null;// 3.$form->submit(['name' =>null ]);assert$form->get('name')->getData() ===null;// 4.$form->submit(['name' =>'' ]);assert$form->get('name')->getData() ==='';// 5.$form->submit(['name' =>'Joe' ]);assert$form->get('name')->getData() ==='Joe';
nameis truely optional here, but is also allowed to be the empty string (that is also different from "name not given")- can be viewed as a submission error, though one could normalize it to
[]
- can be viewed as a submission error, though one could normalize it to
edit: this topic isnt directly related to your PR, but a general: "how does the TextType field work currently and is expected to work ideally". i mean there must be a reasoning to kill the empty string from the model value space of the text type.
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.
@backbone87 Yes, the reasoning is to not break data transformers by passingnull instead of an empty string.
ConcerningTextType this PR does not introduce a "new" way, it just considers the setting ofempty_data which was ignored before.
This is a bug fix, I'm not sure it should go in master though.
ping@webmozart
What is the status of this one ? It's been ready for 3.1 for a while :) |
ping :) |
Thank you@HeahDude. |
…ace` (HeahDude)This PR was merged into the 3.1-dev branch.Discussion----------[Form] Let `TextType` implement `DataTransformerInterface`| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#5906| License | MIT| Doc PR | ~Commits-------7e97fbb [Form] let `TextType` implements `DataTransformerInterface`
This PR was merged into the 2.7 branch.Discussion----------[Form] Hardened form type tests| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | ~This one the PRs to come targeting 2.7 needed to harden the code base before doing some changes in master.It takes care of the form types tests part (even if some other tests will be added later, it will also be easier after it), and unlocks merging#21481.It also back ports tests added in#18357 for the TextType.Since it's very hard to merge branches when modifying tests because of BC, and making every test type extend the base type test would involve many function calls to get the tested type, the function `getTestedType` is no longer abstract and return a constant to override instead, it's much better for performance, I didn't change the call in the base type test to keep BC but I suggest to deprecate it in master. Even if those are tests I really think it is worth keeping BC here.The constants also ease testing in the ecosystem of form related libraries that need to be compatible with Symfony 2.7 and 3. I think using "test" as both prefix and suffix on namespaces, classes and names of the constants should discourage using them in real application code. Since this is just about our test suite, I don't think this should be considered a feature, so tis change should be good for 2.7.Two other PRs will follow to solve conflicts in 2.8 and 3.2.I missed last month patches, I hope I won't this time :).Commits-------8cfc3e9 [Form] Hardened form type tests
…altsov)This PR was merged into the 3.4 branch.Discussion----------[Forms] Fixed TextType empty_data value explanation#SymfonyConHackDaySeesymfony/symfony#18357 that introduced a way to use an empty string in models, so setter does not have to be nullable. Usage example:https://github.com/EnMarche/en-marche.fr/blob/master/src/Form/TypeExtension/TextTypeExtension.php#L28Thanx to@HeahDude for helping me with this PR.Commits-------5cfff92 Fix TextType empty_data value explanation