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

Fix GH-16905: Internal iterator functions can't handle UNDEF properties #16907

Open
wants to merge 6 commits into
base: PHP-8.2
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

No description provided.

@nielsdos nielsdos requested a review from bukka as a code owner November 23, 2024 14:28
@nielsdos nielsdos linked an issue Nov 23, 2024 that may be closed by this pull request
@nielsdos
Copy link
Member Author

Actually, throwing may be more appropriate here...

@nielsdos nielsdos marked this pull request as draft November 23, 2024 14:44
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: May use ZVAL_DEINDIRECT().

@iluuu1994
Copy link
Member

iluuu1994 commented Nov 23, 2024

That said, this behavior is deprecated, so a complicated solution might not be necessary. Throwing until we eventually remove the feature sounds acceptable.

@nielsdos
Copy link
Member Author

If we're following the behavior of foreach, then skipping the unset property would be most appropriate.

I guess this makes sense; I'll see how easy it is to do

@nielsdos
Copy link
Member Author

Made that change, turns out that was quite easy to do.

@nielsdos nielsdos marked this pull request as ready for review November 23, 2024 15:09
ext/standard/array.c Show resolved Hide resolved
}

RETURN_COPY_DEREF(entry);
ia_return_current(return_value, array, true);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's fair, same for key(), let me fix this.

ext/standard/array.c Outdated Show resolved Hide resolved
ext/standard/array.c Outdated Show resolved Hide resolved
@iluuu1994
Copy link
Member

Unfortunately, I found out that current() and key() also implicitly advance iterators for dynamic keys (more precisely, it's the unset() I believe). https://3v4l.org/LKlpQ I don't mind the current solution, but maybe my suggestion to not advance was not the most consistent... I'll let you decide what to do. In reality, I hope nobody relies on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal iterator functions can't handle UNDEF properties
3 participants