Errors and weird results on bad input #62

Closed
pehrlich opened this Issue Oct 25, 2012 · 4 comments

Comments

Projects
None yet
2 participants
Phony.normalize('3')
NoMethodError: undefined method `normalize' for 1:Fixnum

Phony.normalize('ewrg')
NoMethodError: undefined method `normalize' for 1:Fixnum

 Phony.normalize('1ewrg')
 => "1[#<Phony::NationalCode:0x007fbb06bfdab0 @national_splitter=#<Phony::NationalSplitters::Fixed:0x007fbb06bfdb00 @size=3, @zero=nil>, @local_splitter=#<Phony::LocalSplitters::Fixed:0x007fbb06cde470 @format=[3, 14]>, @normalize=true>]" 

Perhaps it should throw its own error class, or simply return the input back again

Owner

floere commented Oct 25, 2012

Thanks for the issue.

Please see the documentation:
https://github.com/floere/phony/blob/master/README.textile#plausibility

So one idea is:
Phony.normalize number if Phony.plausible? number

Also see:
#37
#35
#44
for extensive discussions on why not to return the input, or throw an error.

Cheers!

Owner

floere commented Oct 26, 2012

P.S: If you'd like to know more about the why not – or have some input on why it should behave like you think, please let me know :)

Well, I haven't been through all the discussions, but I was initially somewhat alarmed to see this behavior. Using this as my validator, it is important to have any input catch. Bad input should be reliably thrown back to the user. But as it is a generic exception class I don't (and shouldn't) understand in some cases, and a weird looking thing in other cases, it appears I have no way to do this reliably. From using the code, I would assume that plausible would actually throw the same errors (I guess I figure that plausible would just run normalize and see what happens).

Owner

floere commented Oct 26, 2012

I'm not sure whether my last message helped you.

Bad input should be reliably thrown back to the user.

This can be achieved using a rescue.

The thing is, Phony was made to handle international phone numbers according to E164.

Returning the input back again is problematic as you might not want that number in your database. So if you decide to throw something into the user's face:

Phony.normalize(number) rescue throw_back_to_user(number)

If you still want that number in your DB, you can:

Phony.normalize(number) rescue number

I don't want Phony to make that decision for the user.

Regarding throwing its own error class:
The problem is that it sadly does not really help anybody for the lib to do a generic rescue and packaging the error in a Phony specific error class (without adding any information).

I am aware that it might look alarming at first – but it's just that Phony focuses on handling E164 numbers, which "lerwg" isn't.

To migitate for that, I created the #plausible? method, for people to 100% detect bad numbers (numbers that look correct, but do not actually exist will still get through).

Plausible does not throw errors, but returns true or false, see https://github.com/floere/phony/blob/master/lib/phony/validators.rb#L38-76. (You are right that it runs normalize and sees what happens, but it also does some other fact checking – see some of the specs: https://github.com/floere/phony/blob/master/spec/lib/phony/validations_spec.rb#L80-103)

Cheers,
Florian

floere closed this Jan 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment