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

Use boolean() instead of filled() #68

Merged
merged 2 commits into from
May 11, 2021
Merged

Use boolean() instead of filled() #68

merged 2 commits into from
May 11, 2021

Conversation

reinink
Copy link
Contributor

@reinink reinink commented May 11, 2021

This PR updates the LoginRequest to use the request boolean() method instead of filled() when determining if the user should be remembered.

The primary issue with filled() is that it will return true if remember is false (in a JSON request). This is common when working with Inertia where you have a boolean remember data value.

Using boolean() works for the Blade approach as well, since the underlying FILTER_VALIDATE_BOOLEAN check returns true for the value "on".

@reinink reinink marked this pull request as ready for review May 11, 2021 13:12
@driesvints
Copy link
Member

driesvints commented May 11, 2021

@reinink does the boolean change still works for apps which have already published the stubs?

@reinink
Copy link
Contributor Author

reinink commented May 11, 2021

@driesvints Yup! Using the new boolean() method:

In Blade templates, the remember value is either "on", which resolves to true, or is missing, which resolves to false.

In Inertia page components, the previous transform() done in the Login.vue component basically makes it behave just like the Blade templates, where the remember value is either "on", which resolves to true, or is "" (an empty string), which resolves to false.

Meaning, this should be a non-breaking change for those who have already published the stubs.

@taylorotwell taylorotwell merged commit d1005c8 into laravel:1.x May 11, 2021
@reinink reinink deleted the patch-1 branch May 11, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants