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 #408

Merged
merged 8 commits into from
Dec 15, 2021
Merged

Allow Symfony 6 #408

merged 8 commits into from
Dec 15, 2021

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Jul 18, 2021

  • Dropped Symfony 3.4, 5.0, 5.1 and 5.2 support
  • Added Symfony 6 support

@Kocal Kocal force-pushed the symfony-6 branch 2 times, most recently from 3454cd1 to 686e57f Compare July 18, 2021 08:37
@stof
Copy link
Member

stof commented Jul 19, 2021

Careful about that, as we should avoid releasing a version advocating support for Symfony 6 until the RC releases IMO (as the list of BC breaks is not final yet)

@sanderdlm
Copy link

Friendly reminder 🙂 Anything holding this back from being merged?

.travis.yml Outdated
- php: 8.0
env:
- DEPENDENCIES=dev
- SYMFONY_VERSION='6.0.x-dev'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- SYMFONY_VERSION='6.0.x-dev'
- SYMFONY_VERSION='6.0.*'

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, now I can change to * since Symfony 6 has been released 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-added 6.0.x-dev until Symfony 6.0.1 is released.

@Kocal
Copy link
Contributor Author

Kocal commented Dec 1, 2021

Hum, it looks like I can't type methods to make Symfony 6 happy without breaking Symfony <= 4.. should we drop support of Symfony 3.3 and 4?

@thePanz
Copy link

thePanz commented Dec 2, 2021

@Kocal how is the DoctrineBundle doing in that case? it still supports those versions of SF 🤔
see: https://github.com/doctrine/DoctrineBundle/blob/2.6.x/composer.json

@Kocal
Copy link
Contributor Author

Kocal commented Dec 2, 2021

@thePanz AFAIK they don't have to implement a SerializerInterface or any interfaces that changed between SF 3.x and 6.x.

Before a223ee7, I have the following error with Symfony 6 build:

Declaration of FOS\JsRoutingBundle\Serializer\Normalizer\RoutesResponseNormalizer::normalize($data, $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/travis/build/FriendsOfSymfony/FOSJsRoutingBundle/Serializer/Normalizer/RoutesResponseNormalizer.php on line 25

And after a223ee7, I have the following error with Symfony 3.4/4.4/5.x builds:

Declaration of FOS\JsRoutingBundle\Serializer\Normalizer\RoutesResponseNormalizer::normalize($data, ?string $format = NULL, array $context = Array): array must be compatible with Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize($object, $format = NULL, array $context = Array) in /home/travis/build/FriendsOfSymfony/FOSJsRoutingBundle/Serializer/Normalizer/RoutesResponseNormalizer.php on line 20

Maybe the solution is to create a custom NormalizerInterface that will be dynamic in function of the Symfony version?

@dmaicher
Copy link
Contributor

dmaicher commented Dec 2, 2021

Actually Serializer return types have been relaxed after releasing 6.0 here.

So this will be part of 6.0.1.

Can you test if you can make it work with latest 6.0.x-dev?

@sebheitzmann
Copy link

can you allow also PHP8.1 ?

@mhpcc
Copy link

mhpcc commented Dec 3, 2021

php 8.1 is allowed (^8.0), it just has to be added to travis

@sebheitzmann
Copy link

php 8.1 is allowed (^8.0), it just has to be added to travis

Your right, i'm sorry. Any date for the sf6 support ?

@Chris53897
Copy link
Contributor

Maybe a similar solution to this is possible for symfony 4.4 support. briannesbitt/Carbon#2508 (comment)

In my eyes a new major version php8 only with support for symfony 5.4 and 6.x should be created to lower the burden of the maintainers.

@sebheitzmann
Copy link

Could you create a 3.0 branch with a sf6 support ? This bundle is the last one blocking my project upgrade.

@sanderdlm
Copy link

sanderdlm commented Dec 7, 2021

@Kocal The two remaining errors:

Fatal error:  Declaration of FOS\JsRoutingBundle\Serializer\Denormalizer\RouteCollectionDenormalizer::supportsDenormalization($data, string $type, ?string $format = NULL): bool must be compatible with Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization($data, $type, $format = NULL) in /home/travis/build/FriendsOfSymfony/FOSJsRoutingBundle/Serializer/Denormalizer/RouteCollectionDenormalizer.php on line 18

Fatal error: Declaration of FOS\JsRoutingBundle\Serializer\Denormalizer\RouteCollectionDenormalizer::supportsDenormalization($data, string $type, ?string $format = NULL): bool must be compatible with Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization($data, $type, $format = NULL) in /home/travis/build/FriendsOfSymfony/FOSJsRoutingBundle/Serializer/Denormalizer/RouteCollectionDenormalizer.php on line 18

seem to be related to this change you introduced in a223ee7. You've reverted some of these changes in 40108d5, but not the supportsDenormalization bool return type. Could you also revert the return type on that method? It is also reverted in the Symfony PR that eased up the return types. Afterwards, we wait for Symfony 6.0.1 I guess 🙂

@Bukashk0zzz
Copy link

And Symfony 6.0.1 is here :) Any updates?

@mhpcc
Copy link

mhpcc commented Dec 14, 2021

symfony 6.0.1 has not removed the return type declarations from serializer but the ones in the normalizer interfaces, so this should be ready to be merged

@tobias-93
Copy link
Collaborator

Hi,

Sorry, this bundle has been under my radar for a long time.
Please rebase on the newest master to test this on all dependencies and PHP versions we're currently supporting (using Github Actions).
I'll review it when you've done this so we can finally support SF6.

@Kocal Kocal force-pushed the symfony-6 branch 4 times, most recently from 75b877b to 37428f9 Compare December 14, 2021 23:12
@Kocal
Copy link
Contributor Author

Kocal commented Dec 14, 2021

Hi @tobias-93, no worries! :)

The PR has been rebased on latest master. I've removed Symfony 3.4 and 5.0/5.1/5.2 support since those versions are not maintened anymore.

I would still need help with normalizers params/returns types conflicts.

Copy link
Collaborator

@tobias-93 tobias-93 left a comment

Choose a reason for hiding this comment

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

I think this should fix the forward compatibility given the Symfony Bugfix as mentioned by @dreadnip

Serializer/Normalizer/RouteCollectionNormalizer.php Outdated Show resolved Hide resolved
Serializer/Normalizer/RoutesResponseNormalizer.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@Kocal Kocal force-pushed the symfony-6 branch 2 times, most recently from 2ba57be to a3b7175 Compare December 15, 2021 06:09
@Kocal
Copy link
Contributor Author

Kocal commented Dec 15, 2021

The CI is green, I've literally removed types on (de)normalizers... 😬

I think we can add them back when dropping support of Symfony 4.4 or/and 5.3.

@sebheitzmann
Copy link

Good news. Congrat

Co-authored-by: David Maicher <mail@dmaicher.de>
@stof
Copy link
Member

stof commented Dec 15, 2021

The way to be compatible with all versions is to support only PHP 7.2+ (PHP 7.1 does not support variance rules) and to add the return type in your class (but not the argument type). This way, you are compatible with both versions thanks to variance rules.

@tobias-93 tobias-93 merged commit d9aecb3 into FriendsOfSymfony:master Dec 15, 2021
@tobias-93
Copy link
Collaborator

Thanks @Kocal

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.

10 participants