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

Support backed enums in #[MapQueryParameter] #50910

Closed
kells64000 opened this issue Jul 7, 2023 · 5 comments · Fixed by #51004
Closed

Support backed enums in #[MapQueryParameter] #50910

kells64000 opened this issue Jul 7, 2023 · 5 comments · Fixed by #51004

Comments

@kells64000
Copy link

Symfony version(s) affected

6.3.1

Description

If you use #[MapQueryParameter] and type the property with Enum, the QueryParameterValueResolver only accepts scalars.

If I change the resolver to BackedEnumValueResolver, it only looks at the attributes and not the query.
The enum is never resolved and the value is null.

How to reproduce

Controller:

#[Route('/', name: 'test_param_enum']
public function testParameterAction(#[MapQueryParameter] Status $status = null) 
{
        
}

OR

#[Route('/', name: 'test_param_enum']
public function testParameterAction(#[MapQueryParameter(resolver: BackedEnumValueResolver::class)] Status $status = null) 
{
        
}

Enum:

enum Status: string
{
    case New = 'new';
    case Published = 'published';
}

Query:

/?status=new

Possible Solution

  1. Allow enum in QueryParameterValueResolver
  2. Check for query parameters in BackedEnumValueResolver
  3. Add a new resolver QueryParameterBackedEnumValueResolver
  4. ???

Additional Context

No response

@andersmateusz
Copy link
Contributor

@kells64000
Why do u consider this a bug? In documentation (https://symfony.com/blog/new-in-symfony-6-3-query-parameters-mapper) there is no non-scalar value usage example. According to source code Symfony\Component\HttpKernel\Controller\ArgumentResolver\BackedEnumValueResolver can resolve only Symfony\Component\HttpFoundation\Request::attributes. Considering another Symfony\Component\HttpKernel\ControllerValueResolverInterface implementation i.e. Symfony\Component\HttpKernel\Controller\ArgumentResolver\DateTimeValueResolver there is also only support for attributes:

 if (!is_a($argument->getType(), \DateTimeInterface::class, true) || !$request->attributes->has($argument->getName())) {
     return [];
 }

So I would consider this as feature, not as a bug. The question is whether this feature is needed or not.

@derrabus
Copy link
Member

I agree that this is a new feature. And it makes total sense to support enums here. Do you want to wirk on a PR?

@andersmateusz
Copy link
Contributor

@derrabus
Sure, I can work on this. I propose to change issue name to '#[MapQueryParameter] Add support for \BackedEnum'.

@derrabus derrabus changed the title #[MapQueryParameter] Can't type property with Enum Support backed enums in #[MapQueryParameter] Jul 14, 2023
@kells64000
Copy link
Author

It was a bug for me because why can I use any resolver if none of them looks at the query parameters?

@andersmateusz
Copy link
Contributor

andersmateusz commented Jul 17, 2023

It was a bug for me because why can I use any resolver if none of them looks at the query parameters?

Only QueryParameterValueResolver supports resolving query parameters, besides there are many implementations of ArgumentResolverInterface, but I agree that it is misleading that you can use any ArgumentResolverInterface as argument for #[MapQueryParameter]. I guess authors aimed to allow for custom query parameter value resolver. There are annotations that suggest that you can use only QueryParameterValueResolver for resolving params, although it would be good to make typing more precise e.i. by using intersection types how i described in my PR #51004.

@fabpot fabpot closed this as completed Aug 3, 2023
fabpot added a commit that referenced this issue Aug 3, 2023
…ter]` (andersmateusz)

This PR was merged into the 6.4 branch.

Discussion
----------

[HttpKernel] Support backed enums in `#[MapQueryParameter]`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #50910
| License       | MIT
| Doc PR        | I think documentation about MapQueryParameter is missing

I had two options to introduce this feature. Extending `QueryParameterValueResolver` or extending `BackedEnumValueResolver`. Both options have it's prons and cons. Extending `QueryParameterValueResolver` is not consistent with DRY principle, but on the other hand does not mislead users about usage of `ValueResolverInterface` (`#[MapQueryParameter]`takes `resolver` argument but only `QueryParameterValueResolver` resolves value unless creating your own implementation of the interface).  I have chosen to extend  `QueryParameterValueResolver`. I think in the future separation of concerns should be introduced (resolving query params, resolving attributes etc.). For example by typing resolvers with new interface and using intersection types.

Commits
-------

487f5f8 [HttpKernel] Support backed enums in #[MapQueryParameter]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants