floere / phony

E164 international phone number normalizing, splitting, formatting.
http://florianhanke.com/phony/
MIT License
1.01k stars 226 forks source link

Errors and weird results on bad input #62

Closed pehrlich closed 11 years ago

pehrlich commented 12 years ago
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

floere commented 12 years ago

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: https://github.com/floere/phony/issues/37 https://github.com/floere/phony/issues/35 https://github.com/floere/phony/issues/44 for extensive discussions on why not to return the input, or throw an error.

Cheers!

floere commented 12 years ago

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 :)

pehrlich commented 12 years ago

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).

floere commented 12 years ago

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