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.7] [WIP] Support iterable objects as targets for data_get() #25892

Closed
wants to merge 5 commits into from
Closed

[5.7] [WIP] Support iterable objects as targets for data_get() #25892

wants to merge 5 commits into from

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Oct 3, 2018

A follow-up to the quite old #12638.

  • See commit descriptions for helpful notes.
  • As a reminder, codes if ($value instanceof Collection) { $value = $value->all(); } are not mandatory but for performances (iterate on plain array).

- The underlying Arr::pluck() currently supports iterable objects, so that no change was needed.
- Add tests by copying those for nested arrays, as this code is only called if there are "*" segments.
- It shows that the code is actually simple.
- It removes the "yo-yo calls" between data_get() and Arr::pluck().
- It prevents breakage if the Arr::pluck() implementation changes.
- The method Arr::collapse() being used in data_get(), maybe this change is needed for some target structures in data_get().
- We are just doing the array_merge() manually. Better than nothing.
@vlakoff
Copy link
Contributor Author

vlakoff commented Oct 3, 2018

Mmmm, see how I just removed the Arr::collapse($result) part and things are still working (edit: restored. didn't notice tests were failing actually). Either the tests are incomplete, either it is really unneeded.

Interesting :)

edit: The question is now about support of Traversable in Arr::collapse(). Either it's not needed for support of Traversable in data_get(), either the tests are incomplete.

ping @JosephSilber

Not formal, just some intuition, as:
- the tests were nonetheless passing without the previous commit,
- and data_get() is actually a recursive function.
Didn't notice the tests were broken... The tests should pass again now.

So the situation is:
- either Arr::collapse() doesn't need to support Traversable for them to fully work in data_get(),
- or the tests are incomplete.
@vlakoff vlakoff changed the title [5.7] Support iterable objects as targets for data_get() [5.7] [WIP] Support iterable objects as targets for data_get() Oct 3, 2018
@vlakoff
Copy link
Contributor Author

vlakoff commented Oct 3, 2018

Ok, I have thought more on it, and I'm almost sure there is no need to add support of Traversable in Arr::collapse() for them to be supported in data_get().

Basically, the data_get() function being recursive, we reach the Arr::collapse() at the end, when all subkeys have been iterated and converted to plain arrays (the "foreach { $result[] }" loop).

I have confirmed by trying with some "triple nested" data and some other cases like "several intermediate keys" (e.g. "*.foo.bar.baz.*") or "several consecutive wildcards" (e.g. "foo.*.*.*.bar").

I someone could give confirmation this would be great, then I'll shorten back this PR which is quite simple actually.

@taylorotwell
Copy link
Member

No plans on changing this function.

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.

2 participants