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

[SimpleFormIterator] default to null instead of empty string #8787

Closed
wants to merge 7 commits into from
Closed

[SimpleFormIterator] default to null instead of empty string #8787

wants to merge 7 commits into from

Conversation

kriskw1999
Copy link

@kriskw1999 kriskw1999 commented Mar 31, 2023

The SimpleFormIterator actually is defaulting every input inside a FormDataConsumer to an empty string. So if I have some arrays, objects, or numbers inside, they default to the empty strings when I add new elements to the array.

I propose to make the default equal to null instead. I know this is a kind of breaking change but I think is more consistent, in case we don't have enough information to determine the right return type null should be better.

This should also solve this issue: #8785 because it will default to null when no default value will be provided.

Let me know what you think about it!

@slax57
Copy link
Contributor

slax57 commented Mar 31, 2023

Thanks for submitting this PR.
See my comment here if you don't have already.
We'll be in touch.

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

PR seems good to me, thanks! 🙂
However, for the reasons I explain here #8785 (comment) , we would rather keep this for the next minor version (v4.10).
Hence, can you please rebase your PR on top of the next branch instead?
Thanks!

@kriskw1999 kriskw1999 changed the base branch from master to next April 3, 2023 11:18
@kriskw1999
Copy link
Author

I have done the rebase on the next branch, but comparing it with the next branch I see some extra stuff in the diff. I will open a clean PR with a cherry-pick and close this one. #8792

@kriskw1999 kriskw1999 closed this Apr 3, 2023
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