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

WIP: Allow installation with Symfony 7 #2396

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Nov 6, 2023

I want to use this as a discussion point how we can keep this bundle compatibilty with the latest Symfony Version. The hardest part mostly will be the deprecation of the Symfony FrameworkExtraBundle.

Some background from our side we @sulu use highly the FOSRestBundle and even still use RestRoutingBundle. We are not using any FrameworkExtraBundle so one solution from our side is also skip the ExtraBundle related features when the ExtraBundle is not installed.

Requires

TODO

  • Fix compatibility
  • Decide which min Version for PHP (7.4 would currently be required by the php-cs-fixer shim)
  • Decide what todo with the Symfony FrameworkExtraBundle requirement / features build ontop of it?
    • ParameterConverter -> ArgumentResolver?
    • BodyListener -> normal RequestListener?

Use temporary @mbabker symfony-7 jms serializer branch
Use php-cs-fixer shim to avoid conflicts (requires atleast PHP 7.4)
@mbabker
Copy link
Contributor

mbabker commented Nov 6, 2023

Minimum effort that I think is needed to make this PR mergeable:

  • Add a CI build that uninstalls sensio/framework-extra-bundle and run it on at least Symfony 7.0 (maybe 6.4 as well, there doesn't need to be many builds for this condition but considering the Sensio bundle won't be Symfony 7 compatible having at least that one build is a hard requirement); this is the smoke test to make sure this bundle at least works when SensioFrameworkExtraBundle isn't installed
  • Add checks in FOSRestExtension when enabling the body converter or the view response listener and throw an exception if those are enabled and SensioFrameworkExtraBundle isn't installed (the body converter is hard coupled to the old param converters system and needs to be rewritten as a controller argument resolver, the @View annotation/attribute class extends Sensio\Bundle\FrameworkExtraBundle\Configuration\Template so that entire feature is unusable without the deprecated bundle)

With this, it at least makes this bundle partially functional on Symfony 7, even if a couple of its biggest features aren't available yet. At the same time, though, a part of me wonders if it's even worth the effort to release in that state.

@mbabker
Copy link
Contributor

mbabker commented Nov 6, 2023

  • Decide which min Version for PHP (7.4 would currently be required by the php-cs-fixer shim)

I prefer making decisions based on real life data on this stuff, so let's go to the Packagist stats. The number of installs with PHP 7.2 or 7.3 is so minuscule that bumping to 7.4 should be a no brainer.

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Nov 7, 2023

Like your ideas thank you @mbabker.

With this, it at least makes this bundle partially functional on Symfony 7, even if a couple of its biggest features aren't available yet. At the same time, though, a part of me wonders if it's even worth the effort to release in that state.

From @sulu point of view we plan to Support Symfony 7 for our current @sulu 2.x Version. We only use a very minimal set of the RestBundle features:

all other features are not used by us. So if the decision would be not to Support Symfony 7 in the official Bundle I mostly would cherrypick our used features into a Fork to avoid BC breaks in our minor release. But maybe other projects could benifit if we make a Symfony 7 release and like Sulu get a longer Time to migrate away from FOSRestBundle to directly use the Serializers and avoid BC breaks in there services.

@mbabker
Copy link
Contributor

mbabker commented Dec 29, 2023

I've got a more complete PR at #2400 but unfortunately, a lot of tests have to be disabled because of the FOS\RestBundle\Controller\Annotations\View annotation extending from Sensio\Bundle\FrameworkExtraBundle\Configuration\Template which breaks anything trying to parse annotations/attributes when it runs without sensio/framework-extra-bundle installed. Now to go rewrite that entire component to work without the Sensio bundle 😅

@alexander-schranz
Copy link
Contributor Author

@mbabker when I got some time I can also test @sulu against your branch. There we also dont use the sensio extra bundle.

@alexander-schranz
Copy link
Contributor Author

closing this in favor of #2400

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.

2 participants