[Form] Let TextType implement DataTransformerInterface
#18357
Conversation
HeahDude
commented
Mar 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 to
f274178
Compare
|
Failures unrelated. |
|
This goes in master, right ? |
TextType implements DataTransformerInterfaceTextType implement DataTransformerInterface
|
|
||
| $this->assertSame('', $form->getData()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add a newline at the end of this file. Thanks!
There was a problem hiding this comment.
@javiereguiluz Nice catch! Thanks :)
f274178 to
2a66b4e
Compare
| */ | ||
| public function reverseTransform($data) | ||
| { | ||
| return empty($data) ? '' : $data; |
There was a problem hiding this comment.
Should be null === $data ? '' : $data, no ?
What happens if we submit 0 ?
There was a problem hiding this comment.
@fsevestre Thanks! I've changed it will making the tests and forgot to revert it :)
closes symfony#5906. The submitted data should always be transformed back to the model as a string as NULL in this case could stand for "unset this value" whereas a string property of a class could rely on the string type. Furthermore, this prevents potential issues with PHP 7 which allows type hinting of strings in functions.
2a66b4e to
7e97fbb
Compare
| 'empty_data' => '', | ||
| )); | ||
|
|
||
| $form->submit(null); |
There was a problem hiding this comment.
is there any config with the new system that allows one to distinguish between submit(null) and submit('') in the model data?
There was a problem hiding this comment.
@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 which may 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 a callable getting the form andnull - the final resolution as before anything we must be sure that if 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 data the normalization set it to null :/,
Form::viewToNorm()is actually the default view data transformer,
Conclusion
This PR could add a view data transformer to prevent 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.
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.
@backbone87 Yes, the reasoning is to not break data transformers by passing null instead of an empty string.
Concerning TextType this PR does not introduce a "new" way, it just considers the setting of empty_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 #SymfonyConHackDay See symfony/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#L28 Thanx to @HeahDude for helping me with this PR. Commits ------- 5cfff92 Fix TextType empty_data value explanation