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

[5.2] Strengthen dealing with null keys #12958

Closed
wants to merge 3 commits into from
Closed

[5.2] Strengthen dealing with null keys #12958

wants to merge 3 commits into from

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Apr 1, 2016

Follow-up to #12946.

No behaviour changes, just polishing in order to prevent regressions.

@@ -3080,12 +3080,16 @@ public function setRawAttributes(array $attributes, $sync = false)
/**
* Get the model's original attribute values.
*
* @param string|null $key
* @param null|string $key
Copy link
Member

Choose a reason for hiding this comment

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

please revet that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the other way around, and that's indeed better.

@taylorotwell
Copy link
Member

Isn't this how Arr::get already works though? If the key is null it will just return the array?

@thomasruiz
Copy link
Contributor

@vlakoff
Copy link
Contributor Author

vlakoff commented Apr 1, 2016

Isn't this how Arr::get already works though? If the key is null it will just return the array?

Yeah I've already written this a few times.

It depends if you consider using Arr::get with null key to be a regular use case or on the contrary something to avoid doing if possible. I can keep or remove the relevant commit, your pick.

@taylorotwell
Copy link
Member

It's a regular use case. Passing null as the key returns the entire array.

@thomasruiz
Copy link
Contributor

@taylorotwell Ok I see the issue.
When the model is created, attributes and original are both set to an empty array. However, in PHP, $a = []; // (! $a) === true.
I think the PR should add to the first condition in Arr::get: && ! is_array($array), or just replace the condition altogether with that.

@@ -3082,10 +3082,14 @@ public function setRawAttributes(array $attributes, $sync = false)
*
* @param string|null $key
* @param mixed $default
* @return array
* @return mixed|array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still it would be relevant to apply this phpdoc fix.

@vlakoff vlakoff deleted the null-keys branch April 8, 2016 12:51
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.

4 participants