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

Patch for Kazakhstan phone numbers breaks region resolution in PartialFormatter #752

Closed elitalon closed 7 months ago

elitalon commented 7 months ago

New Issue Checklist

Steps to reproduce

  1. Initialise a PartialFormatter instance with the default region set to "US"
  2. Call formatPartial(_:) with the value "+7"
  3. Call currentRegion
Expected result

currentRegion returns "RU".

Actual result

currentRegion returns "US".

Note that when the default region is set to something other than "US", currentRegion returns "RU" as expected. The following tests demonstrate this behaviour:

    // Success
    func testMinimalRUNumberFromESRegion() {
        let partialFormatter = PartialFormatter(phoneNumberKit: phoneNumberKit, defaultRegion: "ES")
        _ = partialFormatter.formatPartial("+7")
        XCTAssertEqual(partialFormatter.currentRegion, "RU")
    }

    // Failure
    func testMinimalRUNumberFromUSRegion() {
        let partialFormatter = PartialFormatter(phoneNumberKit: phoneNumberKit, defaultRegion: "US")
        _ = partialFormatter.formatPartial("+7")
        XCTAssertEqual(partialFormatter.currentRegion, "RU")
    }

The fact that the same input ("+7") produces two different results depending on the default region seems an inconsistent behaviour. I think this was introduced in #689 by changing the way currentRegion is resolved. If I revert that change, all tests above succeed.

Also, I don't understand why a special case was introduced in PartialFormatter for Kazakhstan only. If I'm not mistaken there are other regions that share the same country code (e.g. "+1" or "+44") and have their own flag.

I think #689 should be reverted until a more robust way of solving that "flag problem" is implemented. Ideally one that works in all cases, not just for Kazakhstan.

I'm happy to help with such implementation, but I would need guidance because my current knowledge of phone number metadata is a bit limited.

So far my initial idea is to start with how currentMetadata is updated in extractCountryCallingCode(_:). Right now the implementation uses mainTerritory(forCode:), but I wonder if filterTerritories(byCode:) could be used instead, picking a candidate based on something like leadingDigits:

let candidateTerritory = metadataManager?
    .filterTerritories(byCode: potentialCountryCode)?
    .first { territory in
        if let leadingDigits = territory.leadingDigits {
            parser!.regex.matchesAtStart(leadingDigits, string: processedNumber)
        } else {
            false
        }
    }
    ?? metadataManager?.mainTerritory(forCode: potentialCountryCode)

Environment

I'm integrating PhoneNumberKit through SPM in an iOS app.

bguidolim commented 7 months ago

Hey @elitalon Thanks for raising this issue.

After some investigation, including a lot of back and forward changes made in multiples PRs, I think I'm finally able to fix this issue regarding flag and partial formatter.

I'm gonna open a PR soon.

elitalon commented 7 months ago

Thanks for the fix, @bguidolim! I upgrade PhoneNumberKit in our project and now it works as expected 👌🏼

Do you think it's still worth pursuing switching regional flags based on leading digits?

bguidolim commented 7 months ago

To be honest, I don't think so. It's quite tricky to make it generic considering so many different patterns you can find around the world. Since it's showing the right flag when the number is complete, I would say it's good enough.

elitalon commented 7 months ago

Makes sense 👍🏼 Thanks!