giggsey / libphonenumber-for-php

PHP version of Google's phone number handling library
https://giggsey.com/libphonenumber/
Apache License 2.0
4.73k stars 464 forks source link

Error when testing Symfony code using the `PhoneNumber` class due to mismatched signature for `__unserialize` #636

Closed IcarusSosie closed 2 months ago

IcarusSosie commented 3 months ago

Hi,

I'm building an API using Symfony and API-Platform. Since phone number validation is crucial to the business logic, and since libphonenumber-for-php is already used by legacy projects, I'm reusing it for this project via odolbeau/phone-number-bundle.

So far so good on the business logic code, but I'm running into an issue writing tests for code using the PhoneNumber class, notably Doctrine entities that have a PhoneNumber property.

Testing through an HttpClientInterface seems to generate a proxy for PhoneNumber that uses the LazyProxyTrait, but also extends PhoneNumber. This leads to this fatal error :

Fatal error: Declaration of Symfony\Component\VarExporter\LazyProxyTrait::__unserialize(array $data): void
must be compatible with libphonenumber\PhoneNumber::__unserialize($data)
in /app/vendor/symfony/var-exporter/LazyProxyTrait.php on line 322

The source of the error is the missing array type-hint on the $data argument for the __unserialize method. According to php.net, the expected signature for it is :

public __unserialize(array $data): void

Is there a reason for omitting the hint, like keeping backwards compatibility ? Otherwise I have a branch with the fix that I can submit.

giggsey commented 3 months ago

Hi,

This library supports all the way back to PHP 5.3, so type hints are not available.

IcarusSosie commented 3 months ago

I'm confused, is there a difference between type-hinting a method argument with array as opposed to a class like PhoneNumber ?

in PhoneNumber.php, line 513, the equals method has its arguments type-hinted, for example :

    public function equals(PhoneNumber $other)
    {
        // [...]     
    }
IcarusSosie commented 3 months ago

Just saw your comment on the PR, disregard my last comment. To be honest, php.net is not super clear about which versions support argument type-hinting :smile:

giggsey commented 3 months ago

To be honest, php.net is not super clear about which versions support argument type-hinting :smile:

Agreed, but to their credit, PHP 5.3 is ancient.

If you can, look at libphonenumber-for-php-lite. It targets modern PHP versions and is a drop in replacement for the core libphonenumber functionality.