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

Reflection API does not match PHP's Reflection API #5890

Open
sebastianbergmann opened this issue Aug 9, 2015 · 9 comments
Open

Reflection API does not match PHP's Reflection API #5890

sebastianbergmann opened this issue Aug 9, 2015 · 9 comments

Comments

@sebastianbergmann
Copy link
Contributor

Discovered via sebastianbergmann/phpunit-mock-objects#228 which is discussed in sebastianbergmann/phpunit-mock-objects#229.

This is just one example of where the Reflection API differs between HHVM and PHP:

<?php
ReflectionClass::export(ReflectionParameter::class);
$ php-7 test.php > php-7.txt
$ /usr/local/src/hhvm/src/hphp/hhvm/hhvm test.php > hhvm.txt
$ diff -u php-7.txt hhvm.txt
--- php-7.txt   2015-08-09 05:47:15.317145284 +0200
+++ hhvm.txt    2015-08-09 05:47:20.830186176 +0200
@@ -1,4 +1,10 @@
-Class [ <internal:Reflection> class ReflectionParameter implements Reflector ] {
+
+Notice: Constant SUNFUNCS_RET_DOUBLE already defined
+
+Notice: Constant SUNFUNCS_RET_STRING already defined
+
+Notice: Constant SUNFUNCS_RET_TIMESTAMP already defined
+Class [ <internal:> class ReflectionParameter implements Reflector, Stringish ] {

   - Constants [0] {
   }
@@ -6,149 +12,94 @@
   - Static properties [0] {
   }

-  - Static methods [1] {
-    Method [ <internal:Reflection, prototype Reflector> static public method export ] {
+  - Static methods [2] {
+    Method [ <internal> static public method export ] {

       - Parameters [3] {
-        Parameter #0 [ <required> $function ]
-        Parameter #1 [ <required> $parameter ]
-        Parameter #2 [ <optional> $return ]
+        Parameter #0 [ <required> $func ]
+        Parameter #1 [ <required> $param ]
+        Parameter #2 [ <optional> $ret = false ]
+      }
+    }
+    Method [ <internal> static private method collectAttributes ] {
+
+      - Parameters [4] {
+        Parameter #0 [ <required> &$attrs ]
+        Parameter #1 [ <required> $class ]
+        Parameter #2 [ <required> $function_name ]
+        Parameter #3 [ <required> $index ]
       }
     }
   }

-  - Properties [1] {
+  - Properties [2] {
+    Property [ <default> public $info ]
     Property [ <default> public $name ]
   }

-  - Methods [21] {
-    Method [ <internal:Reflection> final private method __clone ] {
-
-      - Parameters [0] {
-      }
-    }
-
-    Method [ <internal:Reflection, ctor> public method __construct ] {
+  - Methods [25] {
+    Method [ <internal, ctor> public method __construct ] {

       - Parameters [2] {
-        Parameter #0 [ <required> $function ]
-        Parameter #1 [ <required> $parameter ]
+        Parameter #0 [ <required> $func ]
+        Parameter #1 [ <required> $param ]
       }
     }
-
-    Method [ <internal:Reflection, prototype Reflector> public method __toString ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal, implements Reflector> public method __toString ] {
     }
-
-    Method [ <internal:Reflection> public method getName ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> final public method __clone ] {
     }
-
-    Method [ <internal:Reflection> public method isPassedByReference ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getName ] {
     }
-
-    Method [ <internal:Reflection> public method canBePassedByValue ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method isPassedByReference ] {
     }
-
-    Method [ <internal:Reflection> public method getDeclaringFunction ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method canBePassedByValue ] {
     }
-
-    Method [ <internal:Reflection> public method getDeclaringClass ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getDeclaringClass ] {
     }
-
-    Method [ <internal:Reflection> public method getClass ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getDeclaringFunction ] {
     }
-
-    Method [ <internal:Reflection> public method hasType ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getClass ] {
     }
-
-    Method [ <internal:Reflection> public method getType ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getTypehintText ] {
     }
-
-    Method [ <internal:Reflection> public method isArray ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getTypeText ] {
     }
-
-    Method [ <internal:Reflection> public method isCallable ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method isArray ] {
     }
-
-    Method [ <internal:Reflection> public method allowsNull ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method allowsNull ] {
     }
-
-    Method [ <internal:Reflection> public method getPosition ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method isOptional ] {
     }
-
-    Method [ <internal:Reflection> public method isOptional ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method isVariadic ] {
     }
-
-    Method [ <internal:Reflection> public method isDefaultValueAvailable ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method isDefaultValueAvailable ] {
     }
-
-    Method [ <internal:Reflection> public method getDefaultValue ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getDefaultValue ] {
     }
-
-    Method [ <internal:Reflection> public method isDefaultValueConstant ] {
-
-      - Parameters [0] {
-      }
+    Method [ <internal> public method getDefaultValueText ] {
     }
+    Method [ <internal> public method getDefaultValueConstantName ] {
+    }
+    Method [ <internal> public method getPosition ] {
+    }
+    Method [ <internal> public method getAttribute ] {

-    Method [ <internal:Reflection> public method getDefaultValueConstantName ] {
-
-      - Parameters [0] {
+      - Parameters [1] {
+        Parameter #0 [ <required> $name ]
       }
     }
+    Method [ <internal> public method getAttributes ] {
+    }
+    Method [ <internal> public method getAttributeRecursive ] {

-    Method [ <internal:Reflection> public method isVariadic ] {
-
-      - Parameters [0] {
+      - Parameters [1] {
+        Parameter #0 [ <required> $name ]
       }
     }
+    Method [ <internal> public method getAttributesRecursive ] {
+    }
+    Method [ <internal> public method isCallable ] {
+    }
   }
 }
-

The notices have been reported in #5888.

@paulbiss
Copy link
Contributor

paulbiss commented Aug 9, 2015

I'm not seeing the notices when I run the code snippet, I suspect they're unrelated. The rest seems rather implementation specific (you're reflecting on systemlib and many of the differences are naming related and otherwise invisible). I think at best this is low-pri, but I don't think supporting parity with systemlib parameter names is realistic. (http://3v4l.org/2JkWA)

@sebastianbergmann
Copy link
Contributor Author

Not sure what you think this is about, but HHVM's implementation of ReflectionParameter is missing the hasType() and getType() methods which were introduced in PHP 7.

@paulbiss
Copy link
Contributor

paulbiss commented Aug 9, 2015

Ah, that was not clear from the diff...

@sebastianbergmann
Copy link
Contributor Author

Not from the diff, no. Sorry about that. But from sebastianbergmann/phpunit-mock-objects#229 (comment) linked from the first line of the issue :-)

@SiebelsTim
Copy link
Contributor

Is this equivalent to getTypehintText() and might be just an alias?

@stof
Copy link
Contributor

stof commented Aug 10, 2015

@SiebelsTim it is not equivalent, hasType should return true for any type-hinted argument, and getType should return a ReflectionType object (or null when there is no type).
Note that ReflectionType is also used for return types.
See http://3v4l.org/4hj3X

@Orvid
Copy link
Contributor

Orvid commented Aug 10, 2015

After a bit of checking, hasType should be able to be implemented as quite simply:

public function hasType() {
  return $this->getTypeText() !== '';
}

As I don't see a ReflectionType class defined in HHVM currently, I'll assume that's a bit more complicated to implement.

@stof
Copy link
Contributor

stof commented Aug 10, 2015

@Orvid of course it does not exist currently. It is part of the new PHP 7 API

@JoelMarcey JoelMarcey self-assigned this Oct 1, 2015
@JoelMarcey
Copy link
Contributor

A big crux of this looks to be implementing ReflectionType and ReflectionGenerator if I understand what was added to PHP 7. And a couple of new methods to existing types...

https://github.com/tpunt/PHP7-Reference#reflection-additions

JoelMarcey added a commit that referenced this issue Nov 17, 2015
Summary: This diff add the `ReflectionType` class which provides a bit of information
re: type-hinted parameters and return types. Two new public methods are added to
`ReflectionParameter` and `ReflectionFunctionAbstract` as to check for type-hint
information and the create the actual `ReflectionType`

Part of #5890

Reviewed By: jwatzman

Differential Revision: D2526389

fb-gh-sync-id: 7f6c9066e375c00981dc35e5d921796472d2dbfd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants