Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] RepeatedType should always have inner types mapped#36411
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
479d0f6 to7cfe1feCompare
HeahDude left a comment
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.
Great! I was about to suggest you to create PR to do so, thanks!
It adds complexity to validate the option, but we can easily enforce the expected behavior so I'm 👍 for targeting 3.4 with this fix.
Uh oh!
There was an error while loading.Please reload this page.
| * @param string $configurationKey | ||
| * @dataProvider notMappedConfigurationKeys | ||
| */ | ||
| publicfunctiontestNotMappedInner($configurationKey) |
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.
Then this could be removed.
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.
Or better, adapted to have the same behavior as the other.
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.
done
9af2203 to2c80cb8CompareHeahDude left a comment
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.
Thanks! (minor comment)
| ])) | ||
| ->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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please keep it on one line to follow our standards, this also reduces the diff.
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.
Thought fabbot will complain about 120 symbols line.
2c80cb8 to780e74dComparebiozshock commentedApr 10, 2020
Added doc PR. |
| } | ||
| /** | ||
| * @param string $configurationKey |
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 remove this line
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Tests are not covered by BC promise, we do add those in the code base though.
nicolas-grekasApr 10, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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 :) )
780e74d to728cd66Comparexabbuh commentedApr 11, 2020
Wouldn't it make sense to check if we could make the type work with the |
HeahDude commentedApr 11, 2020
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? |
xabbuh commentedApr 11, 2020
What about something like#36430? |
nicolas-grekas commentedApr 13, 2020
Thank you@biozshock. |
Uh oh!
There was an error while loading.Please reload this page.
Always set mapped=true to override inner type mapped setting.
Throw an exception if inner types of RepeatedType has mapped=false