Skip to content

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Nov 28, 2023

This is an attempt to get the ball rolling at deprecating the param converter integration in favor of the framework's native argument resolvers. It's untested, it does not have 100% feature parity with the existing implementation (some of that being by design since the two systems are different), and it's kind of a Frankenbeast trying to deal with Symfony 5.4 support, but here we are.

Note, this will only support PHP 8. Considering the Sensio bundle works fine on Symfony 5.4 and Symfony 6 requires PHP 8, as well as argument resolvers having only ever supported PHP 8 attributes (#[CurrentUser] exists in Symfony 5.4 but has no @Annotation declaration), I personally don't see that as an issue.

The attribute and resolver design try to take inspiration from the framework's #[MapRequestPayload] attribute and RequestPayloadValueResolver while only re-implementing the functionality in the existing RequestBodyParamConverter. So the validator integration is still opt-in (whereas in the framework if the validator's injected it is always used), and this bundle's serializer bridge continues to be used.

Things that have changed in the workflow:

  • No more "is optional" style conditional aborts, all errors now throw
  • Since an argument value resolver can't provide two separate arguments (the resolver itself enforces only providing one value unless working with a variadic argument), and some of the order of operations logic here is already crazy, a validation error now throws its HTTP exception instead of adding the violation list as another argument for the controller
  • The deserialized payload isn't written to the request attributes

Things that still need to be done if this is usable:

  • Tests
  • Wire services in as appropriate
  • Adapt the bundle config?

@mbabker mbabker force-pushed the argument-resolver branch from a8f5223 to 2dfafdd Compare April 4, 2024 17:54
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.

1 participant