-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
#[Map...] is a mess (rfc) #58662
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
Comments
All those attribute are actually configuring how the parameter should be mapped. the "to what" info comes from the parameter type. and combining those attributes will be hard. For instance, the EntityValueResolver has several ways of looking for the entity based on the request. Forcing it to use a value coming from the caller would not fit this (and similar issue will happen for many other resolvers). |
@stof good point about EntityValueResolver, let's look into it. There are 3 ways for entity lookup:
So first two are about the input we use to search. Last one can be about input, as @ro0NL showed, but mostly about the method we use to search. How we can adapt this resolver to ValueConsumers:
This implies we also want some ValueConsumers to consume a set of fields, like: // "?y=2024&m=10" -> $yearAndMonth == [ "year" => "2024", "month" => "10" ]
#[QueryParam(["y" => "year", "m" => "month"]
array $yearAndMonth Now, this consumed value(s) can be used as defined in MapEntity For complex identifiers: #[QueryParam(["y" => "year", "m" => "month"])]
#[MapEntity(id: [ "year", "month" ])]
MonthlyReport $report Same for mapping (criteria): #[QueryParam(["y" => "year", "m" => "month"])]
#[MapEntity(mapping: [ "year", "month" ])]
MonthlyReport $report OR, you can pass all query/payload params and let the mapping decide, well, how to map them, the old way. #[QueryString]
#[MapEntity(mapping: [ "y" => "year", "m" => "month" ])]
MonthlyReport $report -- #[QueryParam("email")]
#[MapEntity(expr: "repository.findByEmail(value)")]
Client $client |
@klkvsk your proposal misses the fact that in the existing implementation, you don't need any attribute for the EntityValueResolver to produce a result for most common cases. Forcing to add 2 attributes on each controller argument would be a massive regression of DX.
We cannot add support for |
As I said, if no ValueConsumer attributes specified, then the $value passed to resolve() call is null, meaning ValueResolvers should fall back to original logic, i.e. use $request->attributes as before.
It won't conflict, again, due to the fact that we don't use attributes anymore, if ValueConsumer is defined. Examples:#[Route('/posts/{slug}')]
public function getPost(
#[MapEntity(expr: 'repository.getPostBySlug(slug)')]
Post $post,
) This will work as usual. No #[QueryParam] or #[BodyParam] or #[RouteParam], therefore #[Route('/posts/{slug}')]
public function getPost(
#[RouteParam('slug')] // or just #[RouteParam]
#[MapEntity(expr: 'repository.getPostBySlug(slug)')]
Post $post,
) This will work the same, but internally, EntityValueResolver will work with Case with conflict: // GET /posts/<categoryId>/?id=<postId>
#[Route('/posts/{id}')]
public function getPost(
// #[RouteParam('id')] -- optional, EntityResolver will use category ID anyway
Category $category,
#[QueryParam('id')]
#[MapEntity(expr: 'repository.find(id)')]
Post $post,
) Here we explicitly define that we intend to use post ID from query for $post, we do not care about category ID. |
Simplified, changes to EntityValueResolver will look like that: final class EntityValueResolver implements ValueResolverInterface
{
public function resolve(Request $request, ArgumentMetadata $argument, mixed $value = null): array
{
if (is_scalar($value)) {
$inputBag = new ParameterBag([$argument->getName() => $value]);
} elseif (is_array($value)) {
$inputBag = new ParameterBag($inputBag);
} else {
$inputBag = $request->attributes;
}
// next every access to $request->attributes is replaced with $inputBag
// everything else stays the same As for expressions: // was: $variables = array_merge($request->attributes->all(), [
$variables = array_merge($inputBag->all(), [
'repository' => $repository,
'request' => $request,
]); Another method would be to create ParameterBag before resolve() is called, and pass to it: public function resolve(Request $request, ArgumentMetadata $argument, ParameterBag $inputBag = null): array
{
$inputBag ??= $request->attributes;
... Again, $inputBag will be non-null only (!) if ValueConsumer attributes are defined. |
@klkvsk When you want to map parameters from your path to an entity it is recommended to use the same name in the path as for the method argument, which ensure that a conflict not happens. So your conflict example becomes:
|
I opened PR #58709 to work on this. |
Thank you for this suggestion. |
Hello? This issue is about to be closed if nobody replies. |
Description
Mapping attributes for controller arguments are a mix of different concepts.
#[MapEntity], #[MapDateTime] - are "map to what".
#[MapQueryString], #[MapQueryParameter], #[MapRequestPayload], #[MapUploadedFile] - are "map from where".
Moreover, they can not be combined, as such you can not do
Providing a value resolver in most cases would not work either:
The problem is ValueResolvers are hardcoded to check only a single input bag (Entity- and DateTimeValueResolver check only request attributes).
Proposed solution:
Example
The text was updated successfully, but these errors were encountered: