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

Allow URLEncodedForms to decode 'on' value als Bool => true #2698

Merged
merged 4 commits into from Jan 18, 2023

Conversation

bobvoorneveld
Copy link
Contributor

When a HTML form uses a checkbox to check something as a Bool, it sends the 'on' value if checked.
The URLEncodedForms decoder should be able to handle that.

<form method="post">
 <input type="checkbox" name="isActive" checked />
 </form>

struct FormData: Decodable {
    let isActive: Bool
}

let formData = try req.content.decode(FormData.self)

fixes #2444

@0xTim
Copy link
Member

0xTim commented Nov 3, 2021

@bobvoorneveld Thanks for this! Just a note to say it hasn't been forgotten, I just to spend some time to sit down and read the form spec etc to make sure we're all good. But wanted you to know this wasn't getting lost in a void!

@bobvoorneveld
Copy link
Contributor Author

bobvoorneveld commented Nov 4, 2021

NP, keep up the good work!

I'm just merging main because I'm using my own branch as the base of one of my projects.

@bobvoorneveld
Copy link
Contributor Author

bobvoorneveld commented Nov 4, 2021

Reading some documentation on input checkboxes via https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox

If the value attribute was omitted, the default value for the checkbox is on, so the submitted data in that case would be subscribe=on.

I can solve this by adding a value to 1 in the input type. But would be nice to support this behaviour?

Is this the spec that you need to confirm?

https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default-on

Copy link
Member

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

LGTM, spec looks correct

@Joannis
Copy link
Member

Joannis commented Mar 13, 2022

@0xTim or @gwynne any objection to merging this?

@Joannis Joannis added the semver-minor Contains new API label Mar 13, 2022
@Joannis
Copy link
Member

Joannis commented Mar 13, 2022

I also don't know if this is a semver-minor or semver-patch

@Joannis Joannis added semver-patch Internal changes only and removed semver-minor Contains new API labels Jan 18, 2023
@Joannis Joannis merged commit a7d8818 into vapor:main Jan 18, 2023
@VaporBot
Copy link
Contributor

These changes are now available in 4.69.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content decode Bool type?
4 participants