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

[HttpKernel] allow boolean argument support for MapQueryString #54153

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

Jean-Beru
Copy link
Contributor

@Jean-Beru Jean-Beru commented Mar 4, 2024

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #53616
License MIT

Add a $filters argument to the MapQueryString attribute to allow filtering parameters using filter_var function. It will be useful to cast "false", "0", "no" or "off" to false before denormalizing the request data.

EDIT

Add a AbstractNormalizer::FILTER_BOOL context option to cast to a boolean using the filter_var function with the \FILTER_VALIDATE_BOOL filter (see https://www.php.net/manual/fr/filter.filters.validate.php).

MapQueryString will use this context option when denormalizing the query string.
MapPayload also but only with data coming from a form.

See the relative comment: #54153 (comment)

Example:

final readonly class SearchDto
{
    public function __construct(public ?bool $restricted = null)
    {
    }
}

final class MyController
{
    #[Route(name: 'search', path: '/search')]
    public function __invoke(#[MapQueryString] SearchDto $search): Response
    {
        // /search?restricted=
        // "true", "1", "yes" and "on" will be cast to true
        // "false", "0", "no", "off" and "" will be cast to false
        // other will be cast to null
    }
}

@nicolas-grekas
Copy link
Member

I'm not sure how useful this could be: the query string can be a tuple of many different type of values, while this works only of all properties of the DTO are of the same type, isn't it?

@Jean-Beru
Copy link
Contributor Author

I'm not sure how useful this could be: the query string can be a tuple of many different type of values, while this works only of all properties of the DTO are of the same type, isn't it?

The associative array allows to define which parameter is concerned. #[MapQueryString(filters: ['restricted' => \FILTER_VALIDATE_BOOL])] will only apply this filter on the restricted parameter.

TBH, I think that this mapping will mainly be used to cast string to boolean since other types (ex: string to int) are already handled by the Serializer.

@nicolas-grekas
Copy link
Member

Ah OK, so "restricted" is the name of the property. This wasn't obvious to me.

It's a bit of a shame that a type that is already clearly described on the DTO need to be specified again somewhere else.

This maps to the serializer component, which could have a mode to do the cast on its own, don't you think?

@Jean-Beru
Copy link
Contributor Author

Jean-Beru commented Mar 5, 2024

It's a bit of a shame that a type that is already clearly described on the DTO need to be specified again somewhere else.

I totally agree.

This maps to the serializer component, which could have a mode to do the cast on its own, don't you think?

We could pass [FILTER_BOOL => value] in the context to apply this filter. Should this filter be applied by default when using #[MapQueryString] ? Current request parameters like active=false will be cast to false in place of true.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 5, 2024

Yes, that'd be better IMHO. Dunno if bool is the only concerned type of if other ones could benefit from using filter_var to do the casting. Would need to be investigated.
Then yes, I would enable that flag for MapQueryString, but also for MapRequestPayload when the payload is a form submission.

@Jean-Beru Jean-Beru requested a review from dunglas as a code owner March 6, 2024 09:34
@Jean-Beru Jean-Beru changed the title [HttpKernel] allow boolean argument support for MapQueryString [HttpKernel][Serializer] allow boolean argument support for MapQueryString Mar 6, 2024
@Jean-Beru
Copy link
Contributor Author

I updated this PR and its description this way with a new AbstractNormalizer::FILTER_BOOL context option.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that :)

@carsonbot carsonbot changed the title [HttpKernel][Serializer] allow boolean argument support for MapQueryString [HttpKernel] allow boolean argument support for MapQueryString Mar 13, 2024

return filter_var($value, \FILTER_VALIDATE_BOOL, \FILTER_NULL_ON_FAILURE) ?? $value;
return $value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks wrong :)

@fabpot fabpot force-pushed the fix/53616-MapQueryString-with-bool branch from be335e6 to 5c20295 Compare March 17, 2024 14:48
@fabpot
Copy link
Member

fabpot commented Mar 17, 2024

Thank you @Jean-Beru.

@fabpot fabpot merged commit d7d670e into symfony:7.1 Mar 17, 2024
6 of 10 checks passed
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull request Mar 17, 2024
…ryString (Jean-Beru)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

[HttpKernel] allow boolean argument support for MapQueryString

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #53616
| License       | MIT

~~Add a `$filters` argument to the `MapQueryString` attribute to allow filtering parameters using `filter_var` function. It will be useful to cast "false", "0", "no" or "off" to `false` before denormalizing the request data.~~

**EDIT**

Add a `AbstractNormalizer::FILTER_BOOL` context option to cast to a boolean using the `filter_var` function with the `\FILTER_VALIDATE_BOOL` filter (see https://www.php.net/manual/fr/filter.filters.validate.php).

`MapQueryString` will use this context option when denormalizing the query string.
`MapPayload` also but only with data coming from a form.

See the relative comment: symfony/symfony#54153 (comment)

Example:
```php
final readonly class SearchDto
{
    public function __construct(public ?bool $restricted = null)
    {
    }
}

final class MyController
{
    #[Route(name: 'search', path: '/search')]
    public function __invoke(#[MapQueryString] SearchDto $search): Response
    {
        // /search?restricted=
        // "true", "1", "yes" and "on" will be cast to true
        // "false", "0", "no", "off" and "" will be cast to false
        // other will be cast to null
    }
}
```

Commits
-------

5c20295662 [HttpKernel] allow boolean argument support for MapQueryString
symfony-splitter pushed a commit to symfony/serializer that referenced this pull request Mar 17, 2024
…ryString (Jean-Beru)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

[HttpKernel] allow boolean argument support for MapQueryString

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #53616
| License       | MIT

~~Add a `$filters` argument to the `MapQueryString` attribute to allow filtering parameters using `filter_var` function. It will be useful to cast "false", "0", "no" or "off" to `false` before denormalizing the request data.~~

**EDIT**

Add a `AbstractNormalizer::FILTER_BOOL` context option to cast to a boolean using the `filter_var` function with the `\FILTER_VALIDATE_BOOL` filter (see https://www.php.net/manual/fr/filter.filters.validate.php).

`MapQueryString` will use this context option when denormalizing the query string.
`MapPayload` also but only with data coming from a form.

See the relative comment: symfony/symfony#54153 (comment)

Example:
```php
final readonly class SearchDto
{
    public function __construct(public ?bool $restricted = null)
    {
    }
}

final class MyController
{
    #[Route(name: 'search', path: '/search')]
    public function __invoke(#[MapQueryString] SearchDto $search): Response
    {
        // /search?restricted=
        // "true", "1", "yes" and "on" will be cast to true
        // "false", "0", "no", "off" and "" will be cast to false
        // other will be cast to null
    }
}
```

Commits
-------

5c20295662 [HttpKernel] allow boolean argument support for MapQueryString
@antoniovj1
Copy link

Will this fix apply to Symfony 7.0?

@nicolas-grekas
Copy link
Member

It's not a fix but a new feature, so 7.1

@antoniovj1
Copy link

Makes sense, thanks!

@Jean-Beru Jean-Beru deleted the fix/53616-MapQueryString-with-bool branch April 23, 2024 14:31
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MapQueryString doesn't support string value for boolean type
5 participants