odolbeau / phone-number-bundle

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

allow Symfony 6 #96

Closed dmaicher closed 2 years ago

dmaicher commented 2 years ago

TODOs:

dmaicher commented 2 years ago

So actually I think it will be a bit more tricky to support 6.0 and lower versions in parallel :confused:

PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

........................................................SSSSSSS  63 / 139 ( 45%)
PHP Fatal error:  Declaration of Misd\PhoneNumberBundle\Serializer\Normalizer\PhoneNumberNormalizer::normalize($object, $format = null, array $context = []) must be compatible with Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize(mixed $object, ?string $format = null, array $context = []): ArrayObject|array|string|int|float|bool|null in /home/runner/work/phone-number-bundle/phone-number-bundle/src/Serializer/Normalizer/PhoneNumberNormalizer.php on line 68

I think we need two different implementations of PhoneNumberNormalizer...

dmaicher commented 2 years ago

I will look into this in the coming days. We probably can do it similar to here by conditionally declaring the class

dmaicher commented 2 years ago

Now failing because of https://github.com/phpspec/prophecy/issues/527 :confused:

odolbeau commented 2 years ago

Is there any reason to create PhoneNumberNormalizerTrait & LegacyPhoneNumberNormalizerTrait instead of putting methods directly in the right PhoneNumberNormalizer?

dmaicher commented 2 years ago

Is there any reason to create PhoneNumberNormalizerTrait & LegacyPhoneNumberNormalizerTrait instead of putting methods directly in the right PhoneNumberNormalizer?

yeah we cannot put the code from PhoneNumberNormalizerTrait into PhoneNumberNormalizer directly since then the file cannot be parsed with PHP 7.4. That code requires PHP 8.

But I could indeed try to move the code from LegacyPhoneNumberNormalizerTrait into the PhoneNumberNormalizer directly and just keep PhoneNumberNormalizerTrait.

dmaicher commented 2 years ago

Maybe we should actually wait a couple more days.

There is an open discussion about relaxing the typehints on Symfony 6 because this is an issue for other projects as well.

See https://github.com/symfony/symfony/pull/44331

mbabker commented 2 years ago

Could you also add @return annotations in the Twig extension to silence Symfony's debug loader?

Method "Twig\Extension\ExtensionInterface::getFunctions()" might add "array" as a native return type declaration in the future. Do the same in implementation "Misd\PhoneNumberBundle\Twig\Extension\PhoneNumberHelperExtension" now to avoid errors or add an explicit @return annotation to suppress this message.

Method "Twig\Extension\ExtensionInterface::getFilters()" might add "array" as a native return type declaration in the future. Do the same in implementation "Misd\PhoneNumberBundle\Twig\Extension\PhoneNumberHelperExtension" now to avoid errors or add an explicit @return annotation to suppress this message.

Method "Twig\Extension\ExtensionInterface::getTests()" might add "array" as a native return type declaration in the future. Do the same in implementation "Misd\PhoneNumberBundle\Twig\Extension\PhoneNumberHelperExtension" now to avoid errors or add an explicit @return annotation to suppress this message.
mbabker commented 2 years ago

Is there any reason to create PhoneNumberNormalizerTrait & LegacyPhoneNumberNormalizerTrait instead of putting methods directly in the right PhoneNumberNormalizer?

With the normalizer not being final, adding return typehints would be a B/C break since it would force any subclass to add them as well. So it's either use a "creative" solution like this one or just add the types and accept the break.

jderusse commented 2 years ago

things might change in 6.0.1 https://github.com/symfony/symfony/pull/44331

dmaicher commented 2 years ago

I simplified it and reverted changes to PhoneNumberNormalizer since now https://github.com/symfony/symfony/pull/44331 was merged.

So with 6.0.x-dev or 6.0.1 this works fine

Owlympus commented 2 years ago

Hello guys

I have an issue with installation on Symfo 6 (it's a fresh new project)

symfony composer require odolbeau/phone-number-bundle
Using version ^3.6 for odolbeau/phone-number-bundle
./composer.json has been updated
Running composer update odolbeau/phone-number-bundle
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires odolbeau/phone-number-bundle ^3.6 -> satisfiable by odolbeau/phone-number-bundle[v3.6.0].
    - odolbeau/phone-number-bundle v3.6.0 conflicts with odolbeau/phone-number-bundle v3.6.0.
    - symfony/serializer is locked to version v6.0.0 and an update of this package was not requested.

You can also try re-running composer require with an explicit version constraint, e.g. "composer require odolbeau/phone-number-bundle:*" to figure out if any version is installable, or "composer require odolbeau/phone-number-bundle:^2.1" if you know which you need.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.
Nek- commented 2 years ago

@Owlympus this bundle will not be compatible with Symfony 6 until 6.0.1 is released (this should happen in the next weeks). For now you should use Symfony 5.4 if you want to use the bundle.