Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix behavior of PhoneNumberToStringTransformer #1

Merged

Conversation

jankramer
Copy link
Contributor

The current implementation of PhoneNumberToStringTransformer::reverseTransform is incorrect: for each given string it should either return null or an instance of PhoneNumber. If an error occurs while parsing the phone number, the transformer should throw a TransformationFailedException. This will also automatically trigger the Form component to display a validation error.

The fact that it currently returns the input string when it can't parse it leads to strange behavior, especially when mapping to a property that is mapped as a doctrine phone_number type.

The current implementation of
PhoneNumberToStringTransformer::reverseTransform is incorrect: for each
given string it should either return null or an instance of PhoneNumber.
If an error occurs while parsing the phone number, the transformer
should throw a TransformationFailedException. This will also
automatically trigger the Form component to display a validation error.

The fact that it currently returns the input string when it can't parse
it leads to strange behavior, especially when mapping to a property that
is mapped as a doctrine phone_number type.
@jankramer
Copy link
Contributor Author

ping @thewilkybarkid.

@jankramer
Copy link
Contributor Author

ping @misd-service-development

@thewilkybarkid
Copy link
Contributor

Sorry for not looking this sooner.

You right in that the exception should be thrown; the (slight) downside is that it doesn't seem to hit the validation constraint (ie the message returned now is the generic 'This value is not valid' rather than a friendlier 'This value is not a valid phone number'). Not sure if there's a solution...

@jankramer
Copy link
Contributor Author

Right, good point. I think this can be fixed with the invalid_message setting. One moment, I'll try it out and update this PR if it works.

@jankramer
Copy link
Contributor Author

@thewilkybarkid I added the invalid message which is used by the validator extension of the form component to render the error message when transformation failed.

@thewilkybarkid
Copy link
Contributor

👍 Thanks.

thewilkybarkid added a commit that referenced this pull request Oct 9, 2013
Fix behavior of PhoneNumberToStringTransformer
@thewilkybarkid thewilkybarkid merged commit b1823e8 into misd-service-development:master Oct 9, 2013
@jankramer
Copy link
Contributor Author

Thanks for merging! :) While you're at it, could you tag a 1.0.0 release?

@thewilkybarkid
Copy link
Contributor

Sorted! 😃

I had initially been waiting to see if the libphonenumber library was going to survive, seems to be ok!

@thewilkybarkid thewilkybarkid mentioned this pull request Jul 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants