[Form] RepeatedType should always have inner types mapped #36411
Conversation
biozshock
force-pushed
the
repeated-type_not-mapped
branch
2 times, most recently
from
479d0f6
to
7cfe1fe
Compare
src/Symfony/Component/Form/Extension/Core/Type/RepeatedType.php
Outdated
Show resolved
Hide resolved
| * @param string $configurationKey | ||
| * @dataProvider notMappedConfigurationKeys | ||
| */ | ||
| public function testNotMappedInner($configurationKey) |
There was a problem hiding this comment.
Then this could be removed.
There was a problem hiding this comment.
Or better, adapted to have the same behavior as the other.
biozshock
force-pushed
the
repeated-type_not-mapped
branch
2 times, most recently
from
9af2203
to
2c80cb8
Compare
| $builder | ||
| ->addViewTransformer(new ValueToDuplicatesTransformer([ | ||
| $options['first_name'], | ||
| $options['second_name'], | ||
| ])) | ||
| ->add($options['first_name'], $options['type'], array_merge($options['options'], $options['first_options'])) | ||
| ->add($options['second_name'], $options['type'], array_merge($options['options'], $options['second_options'])) | ||
| ->add( |
There was a problem hiding this comment.
Please keep it on one line to follow our standards, this also reduces the diff.
There was a problem hiding this comment.
Thought fabbot will complain about 120 symbols line.
biozshock
force-pushed
the
repeated-type_not-mapped
branch
from
2c80cb8
to
780e74d
Compare
|
Added doc PR. |
| } | ||
|
|
||
| /** | ||
| * @param string $configurationKey |
There was a problem hiding this comment.
Done!
Just a side question in case you have time: Wouldn't such comments ease moving to typehinted scalar types when 3.4 will be EOL and 4.4 will require 7.1?
There was a problem hiding this comment.
Tests are not covered by BC promise, we do add those in the code base though.
There was a problem hiding this comment.
We already added all possible types in 5.0, we should not forget to add it here also that's true; but if we do, that's no big deal (and eventually someone will figure out :) )
biozshock
force-pushed
the
repeated-type_not-mapped
branch
from
780e74d
to
728cd66
Compare
|
Wouldn't it make sense to check if we could make the type work with the |
|
You mean ignoring the option or config setting? It could if we target master with a new feature as the mapping is a requirement for the transformer to do the proper validation. I think that trying to do otherwise will lead to adding unnecessary complexity. Unless you see a simple way to do it? |
|
What about something like #36430? |
nicolas-grekas
changed the title
|
Thank you @biozshock. |
Always set mapped=true to override inner type mapped setting.
Throw an exception if inner types of RepeatedType has mapped=false