Skip to content

[5.6] Let Arr::wrap(null) return empty array#21745

Merged
taylorotwell merged 3 commits into
laravel:masterfrom
TheAlexLichter:feature-array-wrap-empty-array-on-null
Oct 19, 2017
Merged

[5.6] Let Arr::wrap(null) return empty array#21745
taylorotwell merged 3 commits into
laravel:masterfrom
TheAlexLichter:feature-array-wrap-empty-array-on-null

Conversation

@TheAlexLichter
Copy link
Copy Markdown
Contributor

Change Arr::wrap function to return an empty array when providing null as argument. This is the same way Ruby handles it and also more intuitive IMO

@sisve
Copy link
Copy Markdown
Contributor

sisve commented Oct 19, 2017

The semantics of the word "wrap" indicate, to me, that I will get whatever value I pass in wrapped in an array. Sure, there's a special-case for arrays, and that annoys me, but I disagree on adding more of these special cases.

Why shouldn't count(Arr::wrap($value)) === 1 for non-arrays?

@@ -601,6 +601,10 @@ public static function where($array, callable $callback)
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change comment: "If the given value is not an array, wrap it in one." is no longer true in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done ☺️

@TheAlexLichter
Copy link
Copy Markdown
Contributor Author

@sisve I understand your thoughts. In the end, it's a design decision. But for me, it appears to be more useful like Ruby implemented it.

@taylorotwell taylorotwell merged commit b345fdf into laravel:master Oct 19, 2017
@taylorotwell
Copy link
Copy Markdown
Member

I do like matching other ecosystems where possible.

@thecrypticace
Copy link
Copy Markdown
Contributor

This also matches the array cast in PHP. (array) null === []

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.

5 participants