I'm not sure I like this fix, but given the order of operations between the param converters in SensioFrameworkExtraBundle and the controller argument resolvers in the HttpKernel component, this is probably going to cause the least number of headaches for users when upgrading. And in some ways, this is the same problem as#40333 but in reverse.

Because the param converters are triggered on thekernel.controller event, they will handle converting values before the controller argument resolvers are fired. This means that a typehintedDateTimeInterface will potentially be handled twice (once by the param converter, once by the argument resolver). To avoid theTypeError noted in the issue, a practical fix is to gracefully handle a previously converted value in this resolver. As the other options aren't great (removing services from the container or turning off the param converters in full, which has other side effects), this is probably the most practical fix.

carsonbot added Status: Needs Review Bug HttpKernel labels

Apr 28, 2022

carsonbot added this to the6.1 milestone

Apr 28, 2022

nicolas-grekas approved these changes

Apr 30, 2022

View reviewed changes

src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/DateTimeValueResolver.php OutdatedShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 30, 2022

Copy link

Member

chalasr commentedApr 30, 2022•
edited
Loading

or turning off the param converters in full, which has other side effects),

Can you elaborate on the side effects? I'm really not fond of patching Symfony to support a mix of param converters and argument value resolvers

Handle previously converted DateTime arguments

a48ecb6

mbabker force-pushed thedatetime-param-converter-compat branch from5fb1e88 toa48ecb6Compare

May 1, 2022 00:03

Copy link

ContributorAuthor

mbabker commentedMay 1, 2022

or turning off the param converters in full, which has other side effects),
Can you elaborate on the side effects? I'm really not fond of patching Symfony to support a mix of param converters and argument value resolvers

If your app already relies on them, then turning off the param converters (especially with#43854 not making it into 6.1) ends up being a much bigger refactoring. As much as I don't like the patch either, being a pragmatist here, I do think it solves the upgrade issue in a way that causes the least frustration for end-users (as otherwise you really either have to turn off the param converters and rewrite all controllers relying on those or mess with the container to remove one of the services to avoid the double-conversion issue), and this one condition can be re-evaluated or removed once we're at a version where both of theSensioFrameworkExtraBundle converters have replacements.