Skip to content

[HttpKernel] Explain how to define default value in MapQueryString #19627

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

Merged

Conversation

NeilPeyssard
Copy link
Contributor

Hello,

The current example of how using a MapQueryString attribute is not really clear about default DTO value. Some people expects to have a new instance of their DTO.

The goal of this PR is to add a bit of details about how to define this default value.

see #54042

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Thanks Neil!

I think this is correct, but I'll wait for other reviews before merging.

@@ -451,6 +451,24 @@ HTTP status to return if the validation fails::

The default status code returned if the validation fails is 404.

If the request query string is empty and if you still need to have a valid DTO, you can
define a default value in your controller method::
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
define a default value in your controller method::
define a default value to your controller parameter::

Copy link
Member

Choose a reason for hiding this comment

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

I tweaked this while merging ... but apparently, the correct preposition here is for instead of to 😊 Thanks!

Comment on lines +470 to +471
In this case your DTO should have default values.

Copy link
Member

Choose a reason for hiding this comment

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

I think this line may be superfluous 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I removed it while merging. Thanks.

@javiereguiluz javiereguiluz merged commit 6b27438 into symfony:6.4 Mar 12, 2024
@javiereguiluz
Copy link
Member

Neil, thanks a lot for this improvement ... and congrats on your first Symfony Docs contribution 🎉

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.

4 participants