stefanfoulis / django-phonenumber-field

A django model and form field for normalised phone numbers using python-phonenumbers
MIT License
1.49k stars 315 forks source link

Make PhoneNumber.from_string() region check optional #601

Closed Naggafin closed 7 months ago

Naggafin commented 8 months ago

I submitted a PR request for this issue some time ago last year that I thought had been merged into the main branch of this module. Apparently, it wasn't due to a lapse in explanatory input from me. So, I am making another PR with a more in-depth explanation of what this solves this time.

The phonenumber_field.phonenumber.PhoneNumber class has the following method in it:

@classmethod
def from_string(cls, phone_number, region=None) -> Self:
    """
    :arg str phone_number: parse this :class:`str` as a phone number.
    :keyword str region: 2-letter country code as defined in ISO 3166-1.
        When not supplied, defaults to :setting:`PHONENUMBER_DEFAULT_REGION`
    """
    phone_number_obj = cls()
    if region is None:
        region = getattr(settings, "PHONENUMBER_DEFAULT_REGION", None)
    phonenumbers.parse(
        number=phone_number,
        region=region,
        keep_raw_input=True,
        numobj=phone_number_obj,
    )
    return phone_number_obj

This works perfectly fine with either region is manually specified or PHONENUMBER_DEFAULT_REGION is defined within the configuration file. If neither are set though, then this function will always result in a phonenumbers.NumberParseException exception for any number which does not explicitly specify a region code in its string, with the reason being exactly this: invalid region code. That is fine in most cases, but not if you have a number that you do not know the region code to (nor care about) and merely want to validate it purely on a regex basis and get a PhoneNumber object, such as with fixture data for testing, i.e.:

from faker import Faker
from phonenumber_field.phonenumber import PhoneNumber

fake = Faker()
phone = PhoneNumber.from_string(fake.phone_number())  # always results in exception

In a situation like this, we aren't concerned whether or not this is even a real phone number, only that we get a PhoneNumber object from a convincingly-looking phone number string. Currently, there is no way to specify whether or not a region check should take place. The default from phonenumbers.parse() always does. I'm not proposing changing the default behavior, only allowing an option that can do so to better tailor for very particular needs which does not impact regular usage. See diff for proposed changes.

francoisfreitag commented 7 months ago

Previous PR: https://github.com/stefanfoulis/django-phonenumber-field/pull/586

I’m against this idea. The test data generator can do better (e.g. use phonenumbers.example_number()), in order to manipulate realistic data instead of fake (and probably invalid) data. A phone number has special (complex) rules it follows. Just generating something that looks like a phone number is not good for the representativity of a test, as the data generated would most likely be invalid and rejected by validation.

If you want to circumvent the exception, you can use the phonenumber_field.phonenumber.to_python helper, which knows how to handle invalid input.

Barring a use case for actual code (outside of testing), I’m not convinced we should change from_string().

Also, as mentioned in the previous PR:

The docstring of phonenumbers.parse states:

_check_region -- Whether to check the supplied region parameter; should always be True for external callers.

So I would rather not add an argument to allow users to shoot themselves in the foot, especially to help with a testing use case that seems invalid.