Skip to content

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