Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow Symfony 6 #96

Merged
merged 5 commits into from
Dec 7, 2021
Merged

allow Symfony 6 #96

merged 5 commits into from
Dec 7, 2021

Conversation

dmaicher
Copy link

@dmaicher dmaicher commented Nov 25, 2021

TODOs:

  • adjust github action workflow matrix to test against all supported Symfony versions
  • fix PhoneNumberNormalizer so it works for Symfony <6.0 and >=6.0

@dmaicher
Copy link
Author

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

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
Copy link
Author

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

@dmaicher
Copy link
Author

Now failing because of phpspec/prophecy#527 😕

@dmaicher dmaicher marked this pull request as ready for review November 28, 2021 20:36
@@ -67,14 +67,28 @@ public function testValidate($value, $violates, $type = null, $defaultRegion = n
}

if (true === $violates) {
$constraintViolationBuilder = $this->prophesize(ConstraintViolationBuilderInterface::class);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten because of phpspec/prophecy#527

@dmaicher dmaicher force-pushed the symfony_6 branch 2 times, most recently from 964849d to 2c1510b Compare November 28, 2021 21:42
@odolbeau
Copy link
Owner

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

@dmaicher
Copy link
Author

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
Copy link
Author

dmaicher commented Nov 29, 2021

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 symfony/symfony#44331

@mbabker
Copy link

mbabker commented Nov 30, 2021

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
Copy link

mbabker commented Nov 30, 2021

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
Copy link
Collaborator

things might change in 6.0.1 symfony/symfony#44331

symfony-require: 5.3.*
- php: '8.0'
symfony-require: 6.0.*
stability: dev
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stability requirement can be removed once Symfony 6.0.1 was released

@dmaicher
Copy link
Author

dmaicher commented Dec 1, 2021

I simplified it and reverted changes to PhoneNumberNormalizer since now symfony/symfony#44331 was merged.

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

@jderusse jderusse mentioned this pull request Dec 3, 2021
@odolbeau odolbeau merged commit 6fe17a4 into odolbeau:master Dec 7, 2021
@dmaicher dmaicher deleted the symfony_6 branch December 7, 2021 10:01
@Owlympus
Copy link

Owlympus commented Dec 7, 2021

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-
Copy link
Collaborator

Nek- commented Dec 7, 2021

@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.

@Nek- Nek- mentioned this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants