-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Implement iterable_to_array() #3293
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 would love to see this
if (spl_iterator_apply(obj, use_keys ? spl_iterator_to_array_apply : spl_iterator_to_values_apply, (void*)return_value) != SUCCESS) { | ||
zval_ptr_dtor(return_value); | ||
if (zend_is_iterable(iter) == 0) { | ||
zend_type_error("Argument 1 passed to iterable_to_array() must be iterable, %s given", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have this treatment in ZPP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be a type specifier for iterable
currently: http://php.net/manual/en/internals2.funcs.php
Also checked source directly and didn't see anything obvious. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add it :)
Why as a developer should I care about the difference between iterable and Iterator? It would make a lot more sense to me to use |
@iltar iterable is not an iterator, it's a union of iterator or array - polluting existing API didn't seem like a wise decision to me. |
@iltar Change a method signature is a no go for me 👎 |
While I do agree with this additional function (would make a nice addition to 7.3), I believe there's a still a DX issue to have a different implementation, meaning 2 functions that seemingly do the same. I highly doubt that many developers know the differences between them and they won't know when to use which, making it confusing to use. If iterable is |
Maybe, but deprecating core function would require RFC while just adding this self-contained one shouldn't. |
ext/spl/tests/iterable_to_array.phpt
Outdated
|
||
$array = ['a' => 1, 'b' => 2, 'c' => 3]; | ||
$iterator = new ArrayIterator($array); | ||
$generator = function () use ($array) { yield 'a' => 1; yield 'a' => 2; yield 'a' => 3; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use ($array)
seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! I originally had yield from $array;
there. 😉
3563f38
to
018de8b
Compare
Adding a new API seems like a sensible choice - the old one being deprecated later on is probably a good idea over changing existing behaviour |
Hm, since this appears to be controversial, an RFC might be the best way to go, anyway. PS: Considering that a userland implementation of |
Hmm, technically I don't see any correlation between a one-liner and RFC is a must, is this documented somewhere? A one-liner in user-land multiplied by 10000 applications is 10000 duplicated one-liners which will all additionally suffer from PHP has no function autoloading and user-land functions are slow. |
From experience we know that the |
Sorry to be a pain, but we do need a discussion on internals, and an RFC (to follow maybe) ... please get the ball rolling ;) |
RFC declined. |
iterable
type is gaining popularity, unfortunately it is sometimes hard to work with. Especially when the output is needed to be converted into an array, as array_*() functions don't accept iterable, only plain array.The proposal is simple: add
iterable_to_array($iterable)
sibling function toiterator_to_array($iterator)
which will work withiterable
(array or Traversable).Before:
- is_array($iterable) ? $iterable : iterator_to_array($iterable)
After:
+ iterable_to_array($iterable)
This would be really convenient together with
iterable
typehint. 😊Implemented:
iterable_to_array()
iterable_count()