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 constructor param for FilterIterator #1492

Closed

Conversation

danslo
Copy link
Contributor

@danslo danslo commented Jan 4, 2014

When grabbing the constructor arguments through reflection from the
FilterIterator class, HHVM accurately reports the parameter with name
'it' in accordance to both HHVM's FilterIterator.php and PHP's
filteriterator.inc(1).

PHP however, returns 'iterator'. I suspect this is a core PHP bug but I
am running this through you guys to see if this should be reported
upstream or if I am just imagining things.

For your information, the classes that do derive from FilterIterator
already take an 'iterator' param. In any case, attached is a patch
and testcase to make HHVM compatible with PHP, despite it doing crazy stuff.

  1. https://github.com/php/php-src/blob/master/ext/spl/internal/filteriterator.inc#L35

When grabbing the constructor arguments through reflection from the
FilterIterator class, HHVM accurately reports the parameter with name
'it' in accordance to both HHVM's FilterIterator.php and PHP's
filteriterator.inc(1).

PHP however, returns 'iterator'. I suspect this is a core PHP bug but I
am running this through you guys to see if this should be reported
upstream or if I am just imagining things.

For your information, the classes that do derive from FilterIterator
already take an 'iterator' param. In any case, attached is a patch
and testcase to make HHVM compatible with PHP, despite it doing crazy stuff.

1) https://github.com/php/php-src/blob/master/ext/spl/internal/filteriterator.inc#L35
@scannell
Copy link
Contributor

scannell commented Jan 6, 2014

How did you find this? (What code is relying upon it?) If PHP had a spec I'm inclined to agree with @kandy that I wouldn't expect the SPL argument names to be part of it.

@danslo
Copy link
Contributor Author

danslo commented Jan 6, 2014

I found it in the Magento2 codebase and it had to do with Zend code
generation (for a proxy class in this case). This is a very low prio ticket
for me, but was wondering what the appropriate path would be here.

I can give you more specific details this weekend so you can park it for
now.
On Jan 6, 2014 6:04 PM, "Sean Cannella" [email protected] wrote:

How did you find this? (What code is relying upon it?) If PHP had a spec
I'm inclined to agree with @kandy https://github.com/kandy that I
wouldn't expect the SPL argument names to be part of it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1492#issuecomment-31665339
.

@scannell
Copy link
Contributor

scannell commented Jan 6, 2014

I'll park it for now. Does Magento2 have to rely on this and can it instead only depend on sequence? (I think there's going to be a lot of these and given Zend docs != Zend code it's unclear what the "correct" behavior is.)

@danslo
Copy link
Contributor Author

danslo commented Jan 6, 2014

I think it had to do with code generation and having to copy constructor
params (and their names) from a child class of FilterIterator which then no
longer matched their implementation. Something along those lines. I'll keep
you updated.
On Jan 6, 2014 6:44 PM, "Sean Cannella" [email protected] wrote:

I'll park it for now. Does Magento2 have to rely on this and can it
instead only depend on sequence? (I think there's going to be a lot of
these and given Zend docs != Zend code it's unclear what the "correct"
behavior is.)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1492#issuecomment-31668896
.

@scannell
Copy link
Contributor

scannell commented Jan 7, 2014

Per above discussion we've decided not to pull this and we'll fix Magento instead. (The dependency on the parameter names when they don't match the docs is fragile without a language spec -- we're pretty much breaking something either way so no reason to move from the status quo.)

@scannell scannell closed this Jan 7, 2014
@danslo danslo deleted the filteriterator_constructor_param branch January 7, 2014 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants