[Form] Allow NumberToLocalizedStringTransformer::reverseTransform to accept int/float #12470
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@fabpot Is there any chance of getting this merged? |
|
👍 |
|
@webmozart Thanks for having a look at this PR. I'm personally not convinced at all that this is a hack, so if you have the time could you maybe explain why you think that it is? (I tried to figure out what you mean based on #4715 but I'm not sure). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When adding a form type
integerwith aempty_data => 0(aka with a int or float value) the form field will never validate correctly because theNumberToLocalizedStringTransformerwill throw aTransformationFailedExceptionbecause the value is not a string.This PR fixes this problem by allowing int and float types as values in
reverseTransform. Because the string only behavior was enforced by the transformer tests this is a BC change. (Personally I think that this is not a BC break that would cause problems).Another possible fix is to add a normalizer for the
empty_dataoption and transform a int or float to a LocalizedString but that looks/feels like a bad patch job.I hope this PR will have less people going through there validation schema just to figure out that it's the
NumberToLocalizedStringTransformercausing the problem.