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

[Question] Boolean values casted to string #340

Closed
Max13 opened this issue Oct 12, 2020 · 8 comments
Closed

[Question] Boolean values casted to string #340

Max13 opened this issue Oct 12, 2020 · 8 comments
Assignees
Milestone

Comments

@Max13
Copy link

Max13 commented Oct 12, 2020

Considering the following example code:

route('collector.get-messages', {
    body: true,
    collector: 1
});

It's currently (v0.9.4) rendered as: http://example.com/collector/1/messages?body=true

In PHP it's considered to be a string, and both "true" and "false" strictly equals to the boolean value true. I think this comes from the qs dependency stringify() function, if I want to send a PR, should I send it to this repository, or to qs ?

@bakerkretzmar
Copy link
Collaborator

Would $request->boolean('body') in your Laravel app do the trick? That will treat "true" as true and "false" as false.

@Max13
Copy link
Author

Max13 commented Oct 12, 2020

@bakerkretzmar Indeed, while $request->boolean('body') correctly converts it back to strict boolean, the validation rule boolean doesn't.

Generally speaking, boolean true shouldn't be sent as "true"

@bakerkretzmar
Copy link
Collaborator

bakerkretzmar commented Oct 12, 2020

I think for query strings your best bet is to do this formatting yourself. There's no way to actually send it as a boolean because everything in the query string is a string. It looks like people have asked about this in the qs repo and they left it up to users to implement the encoding/decoding they needed: ljharb/qs#91, ljharb/qs#214.

If you want to use the boolean validation rule you could send 1 or "1" instead of true. If you have to send true you could manually convert it to a boolean before validation, either in your controller or in a form request:

$request->merge(['body' => $request->boolean('body')]);

// now $request->input('body') is a boolean

$request->validate(...);

@Max13
Copy link
Author

Max13 commented Oct 13, 2020

@bakerkretzmar The fact that the discussion in still ongoing since 5 years on qs is a proof that it's not an easy answer, even for qs's team

I personally think that the purpose of serialization is to keep at least a boolean value. IMO, laravel/framework's boolean validation rule should support "true" and "false" as strings, but they won't as it's said to be a breaking change in behavior. qs shouldn't transform a boolean to its string representation, but rather to a int representation (as "0" and "1" are the base values we know before the boolean keywords).

Back to Ziggy, being a bridge (I don't mean to reduce Ziggy to "a bridge") between Laravel and qs, should it blindly bridge these 2 projects or should it bridge with sugar syntax? Knowing that Laravel may never validate "true" and "false" strings as being booleans, isn't it Ziggy's responsibility to implement an encoder, translating boolean strings to 0 and 1?

Still IMO, this is an issue 90% of users using boolean validation would face, while I don't see any downside to it

@bakerkretzmar
Copy link
Collaborator

Actually, I think you might be right. My main goal for Ziggy's route() function is for it to work as closely as possible to how Laravel's route() helper does, so I tried this in PHP... and it turns out Laravel's route() helper already does exactly what you're suggesting:

<?php

route('collector.get-messages', [
    'body' => true,
    'collector' => 1,
]);

// returns
"http://example.com/collector/1/messages?body=1"

Unfortunately we'll have to implement our own encoder and decoder to pass to qs, but it looks like we may just be able to ignore all non-boolean values.

If you're already working on a PR for this or you want to, that would be awesome! (Please target the develop branch since this would go into v1). Otherwise I'll look into it when I have time in the next couple weeks.

@bakerkretzmar bakerkretzmar self-assigned this Oct 13, 2020
@bakerkretzmar bakerkretzmar added this to the v1.0 milestone Oct 13, 2020
@Max13
Copy link
Author

Max13 commented Oct 13, 2020

If Laravel’s route didn’t change Boolean values to an int, I would have been surprised ^^’

I would happily submit a PR, I will to read about encoders on qs and submit something.

Thanks for the debate

@Max13
Copy link
Author

Max13 commented Nov 6, 2020

Sorry for the delay, I got stuck on projects and couldn't look at it for now.

@bakerkretzmar
Copy link
Collaborator

No worries! I got it working, it'll be included in v1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants