stefanfoulis / django-phonenumber-field

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

PhoneNumber __str__ method may return non string value #560

Closed HealperDK closed 9 months ago

HealperDK commented 1 year ago

In the PhoneNumber class, the string method is defined as

    def __str__(self):
        if self.is_valid():
            format_string = getattr(settings, "PHONENUMBER_DEFAULT_FORMAT", "E164")
            fmt = self.format_map[format_string]
            return self.format_as(fmt)
        else:
            return self.raw_input

Since self raw_input is a user given input it could assume other values than a string. In my case, it was "None", but I got a __str__ returned non-string (type NoneType) exception, which caused a complete crash in a place that should have only returned a form-invalid. I would argue that replacing the last line with return str(self.raw_input) would be a good fix.

francoisfreitag commented 1 year ago

Hi,

Can you explain how unvalidated user input gets to be formatted as an str? The client can only send an str, they won’t be able to send None. None is a Python thing.

I do agree that __str__ should return an str, however "None" is already associated to None and may cause confusion. Before to change __str__, I’m considering to disallow None as the PhoneNumber value. It makes more sense (the model field would still allow null=True, but would not cast that into a PhoneNumber instance). Disallowing None is what phonenumbers does:

from phonenumbers import parse
>>> x = parse(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/freitafr/dev/django-phonenumber-field/venv/lib/python3.11/site-packages/phonenumbers/phonenumberutil.py", line 2927, in parse
    raise NumberParseException(NumberParseException.NOT_A_NUMBER,
phonenumbers.phonenumberutil.NumberParseException: (1) The phone number supplied was None.
francoisfreitag commented 9 months ago

Not sure what the OP did wrong. The library behavior seems reasonable enough. Closing for inactivity.