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

Add compatibility with symfony/framework-bundle 5.4 #2330

Closed
wants to merge 1 commit into from

Conversation

Th3Mouk
Copy link
Contributor

@Th3Mouk Th3Mouk commented Sep 27, 2021

This PR follow #2332
Tests are currently failling with symfony/routing v6.
In order to be compatible with Symfony framework bundle in version 5.4 without delay, we have to add a conflict rule to avoid the installation of this version, the time to add/ensure a compatibility.

@Th3Mouk Th3Mouk force-pushed the feature/sf54 branch 6 times, most recently from 6dc146a to 27dff47 Compare September 27, 2021 11:07
@mbabker
Copy link
Contributor

mbabker commented Sep 27, 2021

This might be a quick fix, but it's not a good fix IMO.

There are 3 classes using the routing component in this bundle, but the component isn't declared in composer.json in any meaningful way (at a minimum I'd expect it in the require-dev section). There are also no checks to gracefully error out if the component is missing (i.e. an exception in the DI extension if the component isn't available). Granted, it's a bit of a long shot that the routing component wouldn't be installed, but as is, this bundle is relying on transient dependencies to get everything working right.

This would be a good time to make a better long-term fix. If the routing component should be a required dependency, it should be added to the require section in composer.json and then that would be good enough to deal with the Symfony 6 incompatibilities until full support is added. If the routing component is an optional dependency, it should be added to the require-dev section, the conflict section updated to use a good version range (i.e. <4.4, >6 to ensure newer versions of this bundle aren't installed with Symfony 4.3 or earlier), and appropriate checks added to raise an error if someone's trying to use a routing feature and the component isn't installed.

@Th3Mouk
Copy link
Contributor Author

Th3Mouk commented Sep 28, 2021

I'm agree that it's not a definitive fix, but it will allow at short term to have a compatibility. I prefer to find another solution into this two months too. It seems not that difficult to remove some listeners/services at the condition where the symfony/routing is not present dowstream. And probably switching from xml to php format for configuration file will help a lot to do that without too much pass compilers. But I'm not certains this bundle have vocation to be installed without view and routing extra behaviors. I don't know either ATM how the Route attribute will thrown exception to end user, then it will be used without routing package.
Probably adding a hard dependency to symfony/routing is the best way here.

@mbabker
Copy link
Contributor

mbabker commented Sep 28, 2021

I'd just put "symfony/routing": "^4.4|^5.0" into the require section and call it a day. The 3 uses of the routing component in this bundle:

  • The route annotations, which are totally optional and realistically are just offering a shorthand for the methods parameter on the base Route annotation class
  • The AllowedMethodsRouterLoader, which is already an optional integration behind the allowed_methods_listener configuration
  • The concrete ViewHandler class, which is conceivably an optional integration (though really hard to avoid if you're using this bundle's API to build responses in your controllers), but has the UrlGeneratorInterface as a required argument in the constructors; essentially it'd be pretty painful from a B/C perspective to try and make it an optional dependency (a lot of the class would work fine without the router, but the way it's wired in makes it tricky to back it out)

@Th3Mouk
Copy link
Contributor Author

Th3Mouk commented Sep 29, 2021

I come to the same conclusion, what is your recommendation ?

@mbabker mbabker mentioned this pull request Oct 15, 2021
@goetas
Copy link
Member

goetas commented Nov 13, 2021

Closing in favor of #2340

@goetas goetas closed this Nov 13, 2021
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.

None yet

3 participants