Skip to content

Fixes Bug #71412 Incorrect ArrayIterator __construct signature #2566

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

Closed

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Jun 8, 2017

The implementation seems to be what needs to be fixed, not the
documentation.

ArrayIterator doesn't have a getIterator method(),
or an iterator setter,
and I don't think it makes sense for it to have one.
(Not very familiar with this class, though)

The bug in Reflection may have been added in 7.0.2 when fixing #71077
(The fact that the constructor was shared probably predates that)

Notes on BC: This is a constructor, so I don't expect the reflection change to break backwards compatibility.

  • Hopefully, nothing needs to pass a third parameter to ArrayIterator or a subclass. If it does, this may be a problem.

@TysonAndre TysonAndre changed the base branch from master to PHP-7.0 June 8, 2017 04:56
@krakjoe krakjoe added the Bug label Jun 13, 2017
@nikic
Copy link
Member

nikic commented Jun 23, 2017

This is technically BC breaking (because passing more arguments than expected is an error for internal classes), so I'd suggest landing this on master only.

The implementation seems to be what needs to be fixed, not the
documentation.

ArrayIterator doesn't have a getIterator method(),
or an iterator setter,
and I don't think it makes sense for it to have one.
(Not very familiar with this class, though)

The bug **in Reflection** may have been added in 7.0.2 when fixing #71077
(The fact that the constructor was shared probably predates that)
@TysonAndre TysonAndre changed the base branch from PHP-7.0 to master June 23, 2017 16:05
@TysonAndre TysonAndre force-pushed the reflection-arrayiterator-php70 branch from 927176d to 9a0f4ae Compare June 23, 2017 16:06
@krakjoe
Copy link
Member

krakjoe commented Jun 28, 2017

It's not clear whether you meant for this to target 7.2 or master after the 7.2 branch @nikic ?

Whatever, I'll leave this to @remicollet or @sgolemon to merge ... if one of you could take care of that, kthnx.

@sgolemon
Copy link
Member

I think pulling this onto master now for alpha3 is fine. I'll try to carve out a minute to pull this before Tuesday rolls around. :D

@php-pulls
Copy link

Comment on behalf of pollita at php.net:

Closed by 96fe07e

@php-pulls php-pulls closed this Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants