Repository navigation
Conversation
|
Actually, throwing may be more appropriate here... |
iluuu1994
left a comment
There was a problem hiding this comment.
Actually, throwing may be more appropriate here...
If we're following the behavior of foreach, then skipping the unset property would be most appropriate.
| RETURN_FALSE; | ||
| } | ||
|
|
||
| if (Z_TYPE_P(entry) == IS_INDIRECT) { |
There was a problem hiding this comment.
Nit: May use ZVAL_DEINDIRECT().
|
That said, this behavior is deprecated, so a complicated solution might not be necessary. Throwing until we eventually remove the feature sounds acceptable. |
I guess this makes sense; I'll see how easy it is to do |
|
Made that change, turns out that was quite easy to do. |
| } | ||
|
|
||
| RETURN_COPY_DEREF(entry); | ||
| ia_return_current(return_value, array, true); |
There was a problem hiding this comment.
This will implicitly advance when the current element is unset. E.g. with a current(), unset(), current(). I think it would be reasonable for current() not to advance, but to throw if the element is undef.
There was a problem hiding this comment.
I think that's fair, same for key(), let me fix this.
|
Unfortunately, I found out that |
|
Okay, I went back to the "move forward" behaviour as that seems most consistent. |
iluuu1994
left a comment
There was a problem hiding this comment.
LGTM, sorry for the back and forth!
Nah don't worry, I should've checked myself better too ;) |
No description provided.