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.8] Add replace() and replaceRecursive() methods on collections #29088

Merged
merged 2 commits into from Jul 6, 2019
Merged

[5.8] Add replace() and replaceRecursive() methods on collections #29088

merged 2 commits into from Jul 6, 2019

Conversation

@iamgergo
Copy link
Contributor

@iamgergo iamgergo commented Jul 5, 2019

This PR implements the PHP array_replace and the array_replace_recursive behavior on the collections.

With associative array keys, array_merge and array_replace behave the same way. However, with numeric keys/indexes, the merge appends the values to the array, but replace overrides the original value of the specific indexes.

@ahinkle
Copy link
Contributor

@ahinkle ahinkle commented Jul 5, 2019

Resubmit of #20220.

@iamgergo
Copy link
Contributor Author

@iamgergo iamgergo commented Jul 5, 2019

Yes, the functionality is the same, but this PR contains more tests for both functions.

Also, there is a suggestion to use map instead of replace. Well, it can be a good alternative for simple replacements, but for nested and recursive replacements it can be quite tricky.

The other problem with map is that, if the replacement array contains keys, that the original collection does not, we don't really have access to those items. The replace would append the new keys/values to the original collection.

Maybe using map and diff could solve this, but again, if it's a nested replacement, this issue is not solvable simply.

@taylorotwell taylorotwell merged commit 7eb7537 into laravel:5.8 Jul 6, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants