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

[9.x] Added argument transformNullToEmptyString to the functions old() and … #35853

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Conversation

Loots-it
Copy link
Contributor

…getOldInput() and added tests

I implemented this feature: laravel/ideas#1826

Basically I modified the old() and getOldInput() functions to work nicely with the ConvertEmptyStringsToNull middleware. At the moment when you are making an update form (or a create form with a default non empty value) you can't use the old() helper function. Because if a specific field already has a value and the user removes this value in the form, the empty value is overridden with the existing value (or the default value). Which is not the intended behavior and makes the old() function rather useless at the moment. The only way to properly use the old() function at the moment is by not using the ConvertEmptyStringsToNull middleware. But this is a default middleware so in my opinion it makes sense that the old() function can handle this properly.

This shouldn't break anything. I added a default value of false and the tests are all working. I also added some extra tests (asserts) cases that weren't covered yet and added tests for the new feature itself.

Personally I think it would make sense to make the default value equal true. But this would be a breaking change, that's why I used false.

@driesvints driesvints changed the title Added argument transformNullToEmptyString to the functions old() and … [9.x] Added argument transformNullToEmptyString to the functions old() and … Jan 12, 2021
@driesvints
Copy link
Member

I've never had any problems with old() etc in my apps before. I think this should rather be solved in userland by converting null to whatever your app needs.

@GrahamCampbell
Copy link
Member

I agree with @driesvints.

@Loots-it
Copy link
Contributor Author

I've never had any problems with old() etc in my apps before. I think this should rather be solved in userland by converting null to whatever your app needs.

I was going to write a use case where it doesn't work, but by trying to replicate the issue, I found out that I was wrong. I reverted my changes but left the extra asserts that test for the default value and the null case.

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.

None yet

4 participants