Reflection compliance #1600

thekid · 2014-01-19T23:05:40Z

This pull request aims at achieving a greater level of compliance with the original PHP reflection API. It uses these behaviour tests to verify the Reflection classes' functionality.

These bugs are fixed:

ReflectionParameter::canBePassedByValue #1598 - ReflectionParameter::canBePassedByValue()
ReflectionClass::isCloneable() #1596 - ReflectionClass::isCloneable()
Zend incompatibility: ReflectionParameter's function name should accept closure, not only string #1358 - Zend incompatibility: ReflectionParameter's function name should accept closure, not only string (fixed independently by aaf59cb)
Reflection classes should have a __clone method #1572 - Reflection classes should have a __clone method
ReflectionMethod wrong resolution of constants as default parameter value. #1499 - ReflectionMethod wrong resolution of constants as default parameter value.
Behavior diff: ReflectionMethod::getClosure() does not work #1353 - Behavior diff: ReflectionMethod::getClosure() does not work
Behavior diff: ReflectionParameter::isDefault() reports TRUE when optional parameter is before required one #1357 - Behavior diff: ReflectionParameter::isDefault() reports TRUE when optional parameter is before required one
Zend incompatibility: string casting a ReflectionParameter does not produce the expected result #1571 - Zend incompatibility: string casting a ReflectionParameter does not produce the expected result (+ added __toString for all other classes)
ReflectionParameter::getClass() throws exception when parameter is typehinted as 'bool' #1665 - ReflectionParameter::getClass() throws exception when parameter is typehinted as 'bool'
ReflectionMethod->invokeArgs doesn't work on static methods when an instance is passed #1721 - ReflectionMethod->invokeArgs doesn't work on static methods when an instance is passed
HHVM/PHP return different values for \ReflectionFunction::getName() for closures in namespaces #1699 - HHVM/PHP return different values for \ReflectionFunction::getName() for closures in namespaces
When invoking an inherited method with a ReflectionMethod, get_called_class() mistakenly returns the parent class #1728 - When invoking an inherited method with a ReflectionMethod, get_called_class() mistakenly returns the parent class
Reflection API Issues #1770 - Reflection API Issues
Reflection: FilesystemIterator uses unknown class as type-hint #2057 - Reflection: FilesystemIterator uses unknown class as type-hint

Furthermore, these issues are addressed:

Support for parent and self as parameter type hints
Missing ReflectionFunction::inNamespace(), isDeprecated(), getExtension(), getExtensionName()
ReflectionFunction::getStaticVariables() returns string representations of statics, not the actual values
Fix BadMethodCallException for ReflectionClass::getExtensionName()
ReflectionClass::isAbstract() does not return true for traits
ReflectionParameter::__toString should include indicator whether passed by reference
ReflectionFunction does not have a public name member
ReflectionParameter::isDefaultValueConstant() is missing
ReflectionParameter::getDefaultValueConstantName() returns PHP code instead of constant name

There are still differences to the PHP API which I could not or did not address here (e.g. all extension related reflection - HipHop's concept seems to work differently than PHP's, I still haven't quite understood how this could be made to work) and cases in which PHP raises errors, where HipHop throws exceptions for self-consistency.

…ance

. For integers, check if the offset exists, raise a ReflectionException otherwise . For any other type, cast to string, search, raise a ReflectionException if not found

Fixes facebook#1572 Note: Behaves differently than PHP which fatals, using BadMethodCallException here in alignment with "fatal error: call to a method of a non-object"

Set this inside ReflectionFunctionAbstract's constructor, and call into it from both ReflectionFunction and ReflectionMethod

Compliance with PHP

Fixes facebook#1358

Fixes facebook#1596

PHP compliance

when there is no extension, which is the case for userland classes

…esentations PHP compliance

The name member is declared by ReflectionMethod's parent class, ReflectionFunctionAbstract, and thus appears in a different place in the output

Fixes facebook#1598

These functions are implemented in a skeletal way and will always return NULL (or FALSE - anyways, as if there were no extension). This way, we at least no longer get fatal errors though...

Always returns FALSE at the moment This way, we at least no longer get fatal errors though

PHP Compliance

This sucks: PHP returns false instead of throwing an exception, which would be better, and we'd be able to distinguish here from the case where a class constant carries the value FALSE. Nevertheless, be compliant

Still missing: Return by reference, api doc comment, closure handling

…toString()

kandy · 2014-01-20T07:00:14Z

hphp/runtime/ext/reflection/ext_reflection-classes.php

@@ -432,6 +463,11 @@ public function isCallable() {
    return $this->getTypeText() === 'callable';
  }

+  // Prevent cloning
+  public function __clone() {


why not private?

To not raise fatal errors but to be able to throw an exception, in the spirit of $null->method(); also not fatalling in HHVM.

Maks3w · 2014-01-20T07:23:23Z

With the support of parent and self you are fixing #1499 too

Fixes facebook#1499 fpi.phpCode() will already contain resolved fully qualified class names for constants such as `self::CONSTANT` or `ClassName::CONSTANT`; and will contain `CONSTANT` for global constants, both need to be evaluated in the global namespace

We now habe a __toString() implementation in ReflectionFunction Also fix up flaky tests asserting on hardcoded object ids

Indent correctly, at the same time, not like PHP (see thekid/behave@4b0850d)

See facebook#1600 (comment) This parameter is only used to detect whether too many arguments where passed and has no other meaning AFAIS

thekid · 2014-03-16T11:02:04Z

...though no. 3 surely has the least impact:

-.function array_filter($arr = no_args, $func = start("""null"""), $res = start) {
+.function array_filter($arr = no_args, $func = start("""null"""), $res = start("""null""")) {
           .numiters 2;

-# if we get here, a value was supplied for $res
+# if we get here, a value was supplied for $res. NULL is OK.
+          IssetL $res
+          JmpZ start
+

...so I've implemented that for a starter.

$ find hphp/test/ -name 'array_filter*.php' | grep -v bad \|
  xargs ./hphp/test/run hphp/test/slow/reflection/hhas_defaults.php
Running 12 tests in 9 threads
............
All tests passed.

thekid · 2014-03-16T11:04:06Z

I'm seeing failures in default_value_with_self

$ ./hphp/test/run hphp/test/slow/reflection_classes/default_value_with_self.php
Running 1 tests in 1 threads
.
All tests passed.

thekid · 2014-03-16T12:45:33Z

I've found a workaround to the double-namespacing issue which aligns nicely along what was done in 3504e7a to fix #1449 and #1652 but I'm not sure this is the right place

ptarjan · 2014-03-17T04:00:18Z

@thekid I would like to see #1357 fixed anyways, so maybe this is the time. I tried to add your fix internally and @jdelong (correctly) pushed back saying that php totally allows you to have a param without a default value after a param with a default value.

thekid · 2014-03-17T08:27:48Z

Yes, PHP allows this, but reflection then doesn't regard these parameters as optional then, since in reality, you can't omit if from the call:)

As said, it's easy to change HHVM to behave differently, but this is not this pull request's intention:) Alternatively, there's option no. 2, where we change array_filter's signature and remove the third parameter, whose sole purpose is to detect excess arguments.

@elgenie

In #1600 we are actually supporting printing of default args, which evals them. All this stuff uses c-strings all the way down so we need to escape nulls. Reviewed By: @elgenie Differential Revision: D1223776

GrahamCampbell · 2014-03-19T19:16:42Z

I take it this will have to wait until 2.6 now?

ptarjan · 2014-03-19T20:16:59Z

@GrahamCampbell yes, there was just too many issues. I'm still fighting with some and there is a chance of it going into a point release, but the prospects of 2.5 are bad.

GrahamCampbell · 2014-03-19T20:54:16Z

It's a shame. Still, it does fix a lot off issues as well as introducing some...

ptarjan · 2014-03-19T21:25:47Z

@GrahamCampbell right, but more importantly it breaks things that real consumers of HHVM are using already. That can't happen.

GrahamCampbell · 2014-03-19T22:02:15Z

In that case, I'll be looking forward to 2.6 then. :)

thekid · 2014-03-20T07:42:08Z

@ptarjan - let me know if I can do anything (or if that would make things worse:))

ptarjan · 2014-03-20T07:55:25Z

@thekid next time make lots of small PRs that can stand on their own please :) It makes bisecting errors so much easier

thekid · 2014-03-20T07:59:19Z

You're right. This somehow escalated.

GrahamCampbell · 2014-03-20T19:37:19Z

I see that the next release will be 3.0, not 2.5. Would it be possible to delay the 3.0 release a few days so we could get this into it?

ptarjan · 2014-04-08T00:07:17Z

I've been fighting through this and I think I have to declare bankruptcy. Untangling the various issues is very hard on something this large.

I need you (or someone else) to split this up into multiple directed PRs that are segmented and individually tested. There are 67 commits in here, many of which are not tested and break lots of things in subtle ways. I don't know good cut points to easily test. There are lots of new functions added without any tests calling them.

This is the diff I ended up with if you want to start at it and cut down instead: https://gist.github.com/10075701

Ocramius · 2014-04-08T02:36:23Z

@ptarjan all the issues referencing this one as "possible fix" should probably be reopened.

ptarjan · 2014-04-08T04:08:33Z

@Ocramius which ones? The ones referenced in the top are all open except for one which was independently fixed.

Ocramius · 2014-04-08T22:31:40Z

@ptarjan nvm - went through them, they are indeed all open.

BenMorel · 2014-09-22T10:35:11Z

@Ocramius @ptarjan There doesn't seem to be an open issue for the missing ReflectionParameter::isDefaultValueConstant() method, should I create a new one?

Ocramius · 2014-09-22T10:40:09Z

@BenMorel go for it!

thekid added 23 commits January 13, 2014 02:38

Add support for "self"

d71fbd5

Merge remote-tracking branch 'upstream/master' into reflection-compli…

07cdbf5

…ance

Make parameter handling in constructor compliant with PHP

871d283

. For integers, check if the offset exists, raise a ReflectionException otherwise . For any other type, cast to string, search, raise a ReflectionException if not found

Support "parent" as type hint

5d5c3e7

Implement __clone() for all reflection classes

0a32b06

Fixes facebook#1572 Note: Behaves differently than PHP which fatals, using BadMethodCallException here in alignment with "fatal error: call to a method of a non-object"

Ensure ReflectionFunction also has a public name member

e9407d5

Set this inside ReflectionFunctionAbstract's constructor, and call into it from both ReflectionFunction and ReflectionMethod

Show whether a parameter is passed by-ref in __toString()

a6674a3

Compliance with PHP

Support closures as func argument to ReflectionParameter's constructor

38677ea

Fixes facebook#1358

Implement ReflectionClass::isCloneable()

d1a3a71

Fixes facebook#1596

Return true from isAbstract() for traits

3864e1b

PHP compliance

Fix BadMethodCallException ("Call to a member function getName() ...")

da3818c

when there is no extension, which is the case for userland classes

Fix ReflectionClass::isCloneable() when public __clone exists

92feaa6

Make getStaticVariables() return actual values, not their string repr…

cbac0ac

…esentations PHP compliance

Adjust var_dump() expected output

e11d179

The name member is declared by ReflectionMethod's parent class, ReflectionFunctionAbstract, and thus appears in a different place in the output

Implement ReflectionParameter::canBePassedByValue

bc2e8be

Fixes facebook#1598

Implement ReflectionFunction::inNamespace()

9f4d6ec

Implement ReflectionFunction::getExtension[Name]()

7b26bbd

These functions are implemented in a skeletal way and will always return NULL (or FALSE - anyways, as if there were no extension). This way, we at least no longer get fatal errors though...

Implement ReflectionFunction::isDeprecated()

ae57ed7

Always returns FALSE at the moment This way, we at least no longer get fatal errors though

Make getExtension() return NULL for userland classes

b859dc5

PHP Compliance

Return FALSE from ReflectionClass::getConstant() if not found

8924b42

This sucks: PHP returns false instead of throwing an exception, which would be better, and we'd be able to distinguish here from the case where a class constant carries the value FALSE. Nevertheless, be compliant

QA: Coding standards

02ed4f3

Initial implementation of ReflectionFunction::__toString()

e9e7535

Still missing: Return by reference, api doc comment, closure handling

Include api documentation and return by-ref in ReflectionFunction::__…

db56505

…toString()

kandy reviewed Jan 20, 2014
View reviewed changes

thekid added 5 commits January 20, 2014 17:48

Adjust expected output

d88c58c

We now habe a __toString() implementation in ReflectionFunction Also fix up flaky tests asserting on hardcoded object ids

Indicate "Closure" instead of "Function" in __toString()

7d9e8cd

Add Bound variables for closures

307ec23

Indent correctly, at the same time, not like PHP (see thekid/behave@4b0850d)

Implement missing ReflectionParameter::isDefaultValueConstant()

7c8be3c

thekid added 2 commits March 16, 2014 11:56

Make third parameter to array_filter optional

e0cf817

See facebook#1600 (comment) This parameter is only used to detect whether too many arguments where passed and has no other meaning AFAIS

MFH

c2b5680

thekid mentioned this pull request Mar 17, 2014

Reflection: FilesystemIterator uses unknown class as type-hint #2057

Closed

Ocramius mentioned this pull request Mar 17, 2014

Compatibility with HHVM Ocramius/ProxyManager#127

Closed

9 tasks

nubs mentioned this pull request Mar 29, 2014

ReflectionParameter fails with SimpleXmlElement #2222

Closed

scannell added the mid-pri label Apr 2, 2014

ptarjan closed this Apr 8, 2014

thekid deleted the reflection-compliance branch April 9, 2014 06:53

BenMorel mentioned this pull request Sep 22, 2014

ReflectionParameter::isDefaultValueConstant() is missing #3812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reflection compliance #1600

Reflection compliance #1600

thekid commented Jan 19, 2014

kandy Jan 20, 2014

thekid Jan 20, 2014

Maks3w commented Jan 20, 2014

thekid commented Mar 16, 2014

thekid commented Mar 16, 2014

thekid commented Mar 16, 2014

ptarjan commented Mar 17, 2014

thekid commented Mar 17, 2014

GrahamCampbell commented Mar 19, 2014

ptarjan commented Mar 19, 2014

GrahamCampbell commented Mar 19, 2014

ptarjan commented Mar 19, 2014

GrahamCampbell commented Mar 19, 2014

thekid commented Mar 20, 2014

ptarjan commented Mar 20, 2014

thekid commented Mar 20, 2014

GrahamCampbell commented Mar 20, 2014

ptarjan commented Apr 8, 2014

Ocramius commented Apr 8, 2014

ptarjan commented Apr 8, 2014

Ocramius commented Apr 8, 2014

BenMorel commented Sep 22, 2014

Ocramius commented Sep 22, 2014

Reflection compliance #1600

Reflection compliance #1600

Conversation

thekid commented Jan 19, 2014

kandy Jan 20, 2014

Choose a reason for hiding this comment

thekid Jan 20, 2014

Choose a reason for hiding this comment

Maks3w commented Jan 20, 2014

thekid commented Mar 16, 2014

thekid commented Mar 16, 2014

thekid commented Mar 16, 2014

ptarjan commented Mar 17, 2014

thekid commented Mar 17, 2014

GrahamCampbell commented Mar 19, 2014

ptarjan commented Mar 19, 2014

GrahamCampbell commented Mar 19, 2014

ptarjan commented Mar 19, 2014

GrahamCampbell commented Mar 19, 2014

thekid commented Mar 20, 2014

ptarjan commented Mar 20, 2014

thekid commented Mar 20, 2014

GrahamCampbell commented Mar 20, 2014

ptarjan commented Apr 8, 2014

Ocramius commented Apr 8, 2014

ptarjan commented Apr 8, 2014

Ocramius commented Apr 8, 2014

BenMorel commented Sep 22, 2014

Ocramius commented Sep 22, 2014