marmelroy / PhoneNumberKit

A Swift framework for parsing, formatting and validating international phone numbers. Inspired by Google's libphonenumber.
MIT License
5.13k stars 817 forks source link

3.3.3 injecting numbers randomly #445

Closed mmdock closed 1 year ago

mmdock commented 3 years ago

Recently updated from 3.2.0 to 3.3.3

And now there is some randomly inserted numbers happening. (looks like 1534)

This looks to be associated with the nationalPrefixTransformRule in the PhoneNumberMetadata.json

So far, I have only been able to reproduce this behavior with UK

https://user-images.githubusercontent.com/2423465/109885331-3045f700-7c33-11eb-9df0-86e67b8d4c3f.MP4

UPDATE: This line seems to be the cause, but it looks as if it existed in 3.2.0 so I am not sure why this is breaking now: https://github.com/marmelroy/PhoneNumberKit/blob/master/PhoneNumberKit/PhoneNumberParser.swift#L286

Also, in the video, if I type only 1's it inserts 1534, but if I type only 2's it will insert 1481, and again, it is only doing this for the UK. No other country code selected causes this (from the 2 dozen or so i tested anyway)

I have a number of users complaining since updating that this is happening to them, presently going to revert soon, but bringing attention here

UPDATE 2: Was finally able to reproduce it in the US, and the number auto inserted in the front is 268. I have also concluded that this is only happening with versions 3.3.2+ as reverting to 3.3.1 successfully removes this issue

panviktor commented 3 years ago

I confirm, the bug appears on version 3.3.2+, 3.3.1 work fine

george-ovchinnikov commented 3 years ago

I have the same issue

authiatr commented 3 years ago

I'm facing the same issue with the version 3.3.3...

michaelmoneypenny commented 2 years ago

I'm experiencing this issue on 3.3.3, I get the 01481 Guernsey prefix injected into UK mobile numbers if I use 0044 or +44 and omit the leading zero from a mobile number and 01534 if I omit leading zero from a landline.

gtoubassi commented 2 years ago

Seeing as well with 268 getting injected if I type +17812440 into a PhoneNumberTextField. If I print the phoneNumber from that field I get. Note the nationalNumber

PhoneNumber(numberString: "+1 781-2222", countryCode: 1, leadingZero: false, nationalNumber: 2687812222, numberExtension: nil, type: PhoneNumberKit.PhoneNumberType.mobile, regionID: Optional("AG"))

bguidolim commented 2 years ago

Hey!

Whoever is having this issue, could you please check if it is still an issue in the latest version? Keep in mind that if you use Cocoapods, you might need to force the tag since I'm still not able to publish a new version via the official Cocoapod Specs repo.

gtoubassi commented 2 years ago

On the latest (3.4.1) I still see the issue. Did you think it might be fixed or did you just want to confirm this issue is "alive"?

miroslavkovac commented 2 years ago

Yep, still happening with 3.4.1: +44745819 is transformed into +44 1481 745819.

bguidolim commented 2 years ago

I'm trying to reproduce the issue using the sample "AsYouType" with no success. Can anyone provide me a small project with the issue, please?

miroslavkovac commented 2 years ago

@bguidolim here is a snipped to reproduce it:

let phoneNumberKit = PhoneNumberKit()
let parsed = try! phoneNumberKit.parse("+44745819")
let formatted = phoneNumberKit.format(parsed, toType: .international)
print(formatted) // +44 1481 745819
bguidolim commented 2 years ago

@miroslavkovac is it a valid phone number? What should be the expected result? I see the PNK tries to parse using another region with the same country code, that's why the result is more like a guess. Maybe a solution for this issue is simply to avoid the guessing logic? However, there are some regions that share the same country code, which can lead to major issues.

miroslavkovac commented 2 years ago

On its own it is not a valid phone number I guess, but eventually (after adding the final few digits) it will become. The problem is in use case where the app tries to "pretty format" a phone number as user types it. In this case, the library actually says it is a valid number (by not throwing any error when parsing it) and then the app continues by calling format(...) and displays it which adds those random digits in the middle.

I also suspect that the issue is with some regions which share the same county code prefix, so maybe in such case the library can throw some error like ambiguousNumber. In such case the app can fall back to a PartialFormatter which is fine. But without this piece of information/error we had to completely disable the parsing of phone number via PhoneNumebrKit and just use the partial one as we couldn't even tell how many phone numbers/regions are affected.

Thanks for looking into this!

bguidolim commented 2 years ago

Using programmatically, like the example you provided earlier, is supposed to be used with a possible valid phone number only. Checking the sample project "AsYouType" seems to be working fine with the PartialFormatter.

If you're creating your own UI component, please check how PhoneNumberTextField is implemented, there you can see the check if the phone number is not valid, so there is no formatting.

@mmdock, as you are the author of this issue here, please let us know if the discussion here helps you.

miroslavkovac commented 2 years ago

For me phoneNumberKit.isValidPhoneNumber("+44745819") returns true. I will check the PhoneNumberTextField later if that would help with figuring out what is going on.

gtoubassi commented 2 years ago

Our use case is using PhoneNumberTextField during the sign up flow for https://apps.apple.com/app/id1600525061.

We present the user with a PhoneNumberTextField (we don't call withPrefix, just withFlag:true and withExamplePlaceholder true). We also have a "Continue" button on the screen which is disabled. On every keypress we check phoneTextField.isValidNumber to see if the Continue button is enabled. Some of our users, on the way to typing a fully valid US number (e.g. tel:+17812440584), find that they get a temporarily enabled (valid) button at 17812440 where it prepends 268 to the number in that case. I think its decided the 781 thing indicates its Antigua/Barbuda?

If you have time you can download our app and try it real quick to see what I mean. Maybe isValidNumber is not the right thing to call and there is something else we should use? Or perhaps this is technically correct and a good thing to do but we have more users in the boston 781 area code than we have in antigua/barbuda.

Appreciate any advice, and appreciate the library!

authiatr commented 2 years ago

Hello 👋

I was having the issue and I finally found out why.

In my case I was observing some textfield events (.editingDidEnd, .valueChanged, .editingChanged) and every time one of this event was happening I was programmatically reformatting the phone number (textField.phoneNumber) to the E164 format and setting this value to the textField.text property.

The PhoneNumberTextField component is behaving correctly while the user is typing, in my case it was my implantation that was wrong. By the way download the sample project and test the phone numbers. I tried all phone numbers listed here and it worked.

I hope it can help some of you. 🙌

Robin

aaronpride commented 2 years ago

Was able to reproduce this bug and isolate it to changes made in 0a23d5fcad2ae91a020f011369bfbd0758076b56 if you target the sha just before this the bug does not exist.

mmdock commented 1 year ago

so from what i am reading, there are some who believe this to be a bug in the implementation and some who believe it is incorrect usage because developers are using one part of the kit, incorrectly, despite the naming of the function we are using?

djalfirevic commented 1 year ago

@bguidolim @mmdock I've checked both example apps with the same snippet of code which @miroslavkovac wrote earlier and the bug is easily reproducible:

So in ViewController.swift, at the end of viewDidLoad if you put:

let phoneNumberKit = PhoneNumberKit()
let parsed = try! phoneNumberKit.parse("+44745819")
let formatted = phoneNumberKit.format(parsed, toType: .international)
print(formatted) // +44 1481 745819

PhoneBook example app is going to crash, sending an exception notANumber, and AsYouType example app is not going to crash but - it will inject these randomly generated numbers.

bguidolim commented 1 year ago

Hey everyone! I just want to inform you that I'm looking into this issue again.

Let me summarize what I have so far:

  1. Keep in mind that the library is not on the official Cocoapods repo anymore, so you must hardcode the URL on your Podfile like the example below to fetch the latest version:

    pod 'PhoneNumberKit', :git => 'https://github.com/marmelroy/PhoneNumberKit'

    I see that some of you are probably not using the latest version, which leads us to the second item on this list.

  2. The AsYouType example seems to be fixed already with the issue of adding random numbers, and the PhoneBook example is not crashing with the phone number sample provided here, but still adding numbers due to the guessing strategy.

  3. As I mentioned before, using the PNK programmatically implies that, in theory, you must provide a valid number to be parsed.

  4. I'll be pushing a new PR soon, on this PR I'm following @miroslavkovac idea and possibly throwing an ambiguousNumber error. In order to not break everything, I'm keeping the guessing logic, but changing the way the ParseManager is giving it back. From now on, if the guessing logic returns a single value, it will be returned as a parsed number. But if the guessing logic returns more than 1 result, then it's throwing an error with a list of possible numbers that the developer can use to do something or simply ignore.

Any feedback is appreciated.

lukaskukacka commented 1 year ago

Hi @bguidolim, thank you for the fix.

I had a look at the reported problem of inserted random numbers on formatting and I believe I found the root case. I believe there was a regression introduced in version 3.3.2 (3.3.1...3.3.2) by commit 295068f

After that commit an error stopped being thrown for unrecognized numbers and it was just returning the reported "randomly modified" result string.

Good news is your fix #583 seems to fix it again by putting a proper handling back according to my testing 👏 Your fix throws error again, just a different one. Important is it does no longer produce corrupted results.


I made the testing using the following code, running on different commits...

func testRandomInsertions() {
    let incompleteUKNumber = "+44745819"

    /// Try partial formatter - always works fine
    let partialFormatter = PartialFormatter(withPrefix: true)
    print("PARTIAL:", partialFormatter.formatPartial(incompleteUKNumber))

    // Try "full" formatter - inserts random numbers since commit `295068f4fcc1dbf292bc285449da17f7fbbb1469`
    do {
        let phoneNumberKit = PhoneNumberKit()
        let parsed = try phoneNumberKit.parse(incompleteUKNumber)
        let formatted = phoneNumberKit.format(parsed, toType: .international)
        print("FULL:", formatted)
    } catch {
        print("FULL ERROR: \(error)")
    }
}

All good until 3.3.1

Commit 39a8ccc0d90e544a36eee9649ee293efd9857646

PARTIAL: +44 7458 19
FULL ERROR: notANumber

The "random numbers inserted" problem appeared since 3.3.2

Commit 295068f4fcc1dbf292bc285449da17f7fbbb1469

PARTIAL: +44 7458 19
FULL: +44 1481 745819 # !!! Regression: no longer throws an error here, inserts random numbers instead

Suggested fix in #583

on commit d1b7c120fb0889f63ac573957ebf8bc92d513357

PARTIAL: +44 7458 19
FULL ERROR: ambiguousNumber(phone...
bguidolim commented 1 year ago

Wow @lukaskukacka

Thanks for this great report. I guess it's all clear now. I'll merge the PR and release a new version.

Thanks to everyone for this valuable discussion.

mmdock commented 1 year ago
Screen Shot 2023-03-15 at 6 57 36 PM

@bguidolim i actually just finally revisited this as I saw it seemed to be fixed. went to update from 3.3.1, but in the most recent version i can find at least one case where this is still occurring. Made a new issue regarding this here

Surferdude667 commented 8 months ago

This still seems to be a problem in 3.7? I get injected +44 1534

aleksandraspirovska commented 5 months ago

I also reproduce this issue with "3.7" with region Jersey "JE", I get injection of "1534". Also with region Guernsey "GG" I get injection of "1481". It happens after I enter six digits and validate.