phone icon indicating copy to clipboard operation
phone copied to clipboard

valid? resets input parameter

Open martinmalek opened this issue 11 years ago • 15 comments

Whenever I send a parameter which is not a valid phone number Phoner::Phone.valid?(myParam) it is reset and returned as ""

Example: myParam = "ABC123" Phoner::Phone.valid?(myParam) => false myParam is now ""

martinmalek avatar Jan 30 '14 12:01 martinmalek

Thanks for the report @martinmalek. I consider this a bug.

elskwid avatar Apr 26 '14 19:04 elskwid

Hi @martinmalek, I released v1.3.0.beta0 today with some tests around the issue that you've reports. I can't reproduce them here. Is there something I'm missing?

elskwid avatar Apr 27 '14 03:04 elskwid

A little more information, there was a huge issue with the extension extraction code mutating the parameters. It's possible that this commit fixes the issue you've been seeing.

elskwid avatar Apr 27 '14 04:04 elskwid

I'm on 1.2.3.

Seeing this issue.

> raw_number
=> "1-801-848-3641 x02104"
> Phoner::Phone.valid?(raw_number)
=> true
> raw_number
=> "1-801-848-3641"

Going to see if 1.3.0 fixes this.

hoguej avatar Sep 11 '14 06:09 hoguej

Don't know if this is the same bug or a new one, but still getting this result after updating to 1.3.0beta0

hoguej avatar Sep 11 '14 06:09 hoguej

Wow, not the only function that has that issue. This also modifies raw_number... that's terrible.

 Phoner::Phone.parse(raw_number)

hoguej avatar Sep 11 '14 06:09 hoguej

@hoguej Took me long enough!!!

I don't believe that the parameter modification is an issue. There are tests to explicitly test for the modification when using #valid? and I just added one (a6e6ab0ce00) to make sure #parse is safe.

elskwid avatar Jan 30 '15 07:01 elskwid

For anyone who is using an older version of the gem (or until this is fixed in the most recent release) this worked for me. EDIT: it should go in an initializer.

module Phoner
  class Phone
    def self.parse_with_duplication(string, options = {})
      parse_without_duplication(string.try(:dup), options)
    end

    singleton_class.alias_method_chain :parse, :duplication
  end
end

calebanderson avatar Aug 19 '15 20:08 calebanderson

@calebanderson have you tried using the beta?

elskwid avatar Aug 19 '15 20:08 elskwid

I haven't, but the sub! in line makes me think it could still be an issue: https://github.com/carr/phone/blob/v1.3.0.beta0/lib/phone.rb#L192

calebanderson avatar Aug 19 '15 20:08 calebanderson

@calebanderson good call! I fixed that and never pushed out a new release. I can release a new beta later - any chance you can try with master?

elskwid avatar Aug 19 '15 20:08 elskwid

@elskwid Sure I'll give it a shot. I've already looked over the code quite a bit and everything looked good.

calebanderson avatar Aug 19 '15 20:08 calebanderson

@elskwid Everything I checked out seems to be working great, at least as far as I can tell given my little knowledge about international phone numbers.

calebanderson avatar Aug 19 '15 21:08 calebanderson

@calebanderson I will push out a new version as soon as I get a chance (tonight or tomorrow morning). Will keep you posted. Thanks so much!!

elskwid avatar Aug 19 '15 21:08 elskwid

@calebanderson took longer than expected (what doesn't??!?!) but v1.3.0.beta1 is out for you. If it works I'll push out the final version in the days to come.

elskwid avatar Aug 24 '15 02:08 elskwid