odolbeau / phone-number-bundle

Integrates libphonenumber into your Symfony application
MIT License
217 stars 43 forks source link

throw error during serialization when send big data #178

Open Lochar opened 2 months ago

Lochar commented 2 months ago

Hello,

During the test phase of my application, I had an error when I sent a very long data.

When the deserialization tries to hydrate my object, it generates this error: NumberParseException > UnexpectedValueException (The string supplied is too long to be a phone number.)

The constraints are not used, because the application crashes before. I added a Length constraint of 20 just for the example. And the controller is empty for the same reason.

Is it intentional to throw this error at this point in the process?

Here is an example to reproduce (Symfony) : PhoneController

<?php

namespace App\Controller;

use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
use Symfony\Component\Routing\Annotation\Route;

class PhoneController
{
    #[Route('phone', methods: [Request::METHOD_POST])]
    public function __invoke(
        #[MapRequestPayload]
        PhoneDto $dto,
    ): JsonResponse {}
}

PhoneDto

<?php

namespace App\Controller;

use libphonenumber\PhoneNumber;
use Misd\PhoneNumberBundle\Validator\Constraints\PhoneNumber as AssertPhoneNumber;
use Symfony\Component\Validator\Constraints\Length;

class PhoneDto
{
    public function __construct(
        #[AssertPhoneNumber(type: [AssertPhoneNumber::MOBILE])]
        #[Length(max: 20)]
        public PhoneNumber $phone,
    ) {}
}

Sended data :

{
    "phone": "010203040506070809010203040506070809"
}
maxhelias commented 2 months ago

Hi! It doesn't depend on the bundle but on the lib, and it looks intentional, see: https://github.com/giggsey/libphonenumber-for-php/blob/master/src/PhoneNumberUtil.php#L26-L29

Lochar commented 2 months ago

Thanks for your response.

Yes I had indeed found where the error was originally coming from, but when deserializing, I thought it might be more interesting not to re-encapsulate the error with an UnexpectedValueException. But to return null (or something else), in order to let the Symfony validation process continue and finally return a ValidationFailedException.

maxhelias commented 2 months ago

Oh! I see what you mean, the MapRequestPayload deserializes before the validation of the data and the lib raises an exception during the parse in the denormalize well before the validation.

Returning null is not the solution, as it will bypass the validation process and result in behavior that is not what was expected. And if we return the value initially requested, it won't work in some cases where the validation constraint isn't used...

maxhelias commented 2 months ago

A good solution would be to add an option in the context to ignore or not the exception.

Lochar commented 2 months ago

Yes, there you have it, you understood my problem :)

Context could be part of the solution indeed, but we still need to find what the deserialize function could return instead.

I just tried this solution:

try {
    return $this->phoneNumberUtil->parse($data, $this->region);
} catch (NumberParseException) {
    $phoneNumber = new PhoneNumber();
    $phoneNumber->setRawInput($data);

    return $phoneNumber;
}

It seems to work in my case, but I don't think I know and have tested all the use cases.