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

valid? resets input parameter #53

Open
martinmalek opened this issue Jan 30, 2014 · 15 comments
Open

valid? resets input parameter #53

martinmalek opened this issue Jan 30, 2014 · 15 comments
Assignees

Comments

@martinmalek
Copy link

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 ""

@elskwid
Copy link
Collaborator

elskwid commented Apr 26, 2014

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

@elskwid elskwid added the bug label Apr 26, 2014
@elskwid
Copy link
Collaborator

elskwid commented Apr 27, 2014

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
Copy link
Collaborator

elskwid commented Apr 27, 2014

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.

@hoguej
Copy link

hoguej commented Sep 11, 2014

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
Copy link

hoguej commented Sep 11, 2014

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
Copy link

hoguej commented Sep 11, 2014

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

 Phoner::Phone.parse(raw_number)

@elskwid
Copy link
Collaborator

elskwid commented Jan 30, 2015

@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 (a6e6ab0) to make sure #parse is safe.

@calebanderson
Copy link

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

@elskwid
Copy link
Collaborator

elskwid commented Aug 19, 2015

@calebanderson have you tried using the beta?

@calebanderson
Copy link

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

@elskwid
Copy link
Collaborator

elskwid commented Aug 19, 2015

@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?

@calebanderson
Copy link

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

@calebanderson
Copy link

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

@elskwid
Copy link
Collaborator

elskwid commented Aug 19, 2015

@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
Copy link
Collaborator

elskwid commented Aug 24, 2015

@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 elskwid self-assigned this Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants