Implement #38992: invoke() and invokeArgs() static method calls should match

We don't want ReflectionMethod::invoke() to simply ignore its first argument,
if the method to invoke is a static method. Instead we match its ZPP with
that of ReflectionMethod::invokeArgs(). Furthermore, we apply the DRY
principle by factoring out the code to a common helper function to prevent
inadvertent future divergence of the implementations of both methods.

As can be seen from the necessity to adapt some test cases, this causes a
BC break for some pathological cases. Therefore we apply this patch to PHP
7.1 only, which is still in beta phase.

diff --git a/NEWS b/NEWS
index be6eefa0505e89e59cefc44fca77c5105a06960f..fd78f515c4085bb898aba5c995e33b202cb21d32 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,10 @@ PHP                                                                        NEWS
   . Fixed bug #72762 (Infinite loop while parsing a file with opcache enabled).
     (Nikita)
 
+- Reflection:
+  . Implemented request #38992 (invoke() and invokeArgs() static method calls
+    should match). (cmb)
+
 - Standard:
   . Fixed bug #55451 (substr_compare NULL length interpreted as 0). (Lauri
     Kenttä)

diff --git a/UPGRADING b/UPGRADING
index e24d589d47bbcc484f55eb324cc904af85470561..dda21dcc96a5ac1d22e4e531f7fb54f5a2d7e961 100644 (file)
--- a/UPGRADING
+++ b/UPGRADING
@@ -78,6 +78,11 @@ PHP 7.1 UPGRADE NOTES
 - OpenSSL:
   . Dropped sslv2 stream.
 
+- Reflection:
+  . The behavior of ReflectionMethod::invoke() and ::invokeArgs() has been
+    aligned, what causes slightly different behavior than before for some
+    pathological cases.
+
 ========================================
 2. New Features
 ========================================

diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c
index ef04ecdb98589f0690abc65e195f0c391448435c..1be2a3680db3e915f29b8b3ac8325738b3557881 100644 (file)
--- a/ext/reflection/php_reflection.c
+++ b/ext/reflection/php_reflection.c
@@ -3185,19 +3185,18 @@ ZEND_METHOD(reflection_method, getClosure)
 }
 /* }}} */
 
-/* {{{ proto public mixed ReflectionMethod::invoke(mixed object, mixed* args)
-   Invokes the method. */
-ZEND_METHOD(reflection_method, invoke)
+/* {{{ reflection_method_invoke */
+static void reflection_method_invoke(INTERNAL_FUNCTION_PARAMETERS, int variadic)
 {
        zval retval;
-       zval *params = NULL;
-       zend_object *object;
+       zval *params = NULL, *val, *object;
        reflection_object *intern;
        zend_function *mptr;
-       int result, num_args = 0;
+       int i, argc = 0, result;
        zend_fcall_info fci;
        zend_fcall_info_cache fcc;
        zend_class_entry *obj_ce;
+       zval *param_array;
 
        METHOD_NOTSTATIC(reflection_method_ptr);
 
@@ -3221,113 +3220,25 @@ ZEND_METHOD(reflection_method, invoke)
                return;
        }
 
-       if (zend_parse_parameters(ZEND_NUM_ARGS(), "+", &params, &num_args) == FAILURE) {
-               return;
-       }
-
-       /* In case this is a static method, we should'nt pass an object_ptr
-        * (which is used as calling context aka $this). We can thus ignore the
-        * first parameter.
-        *
-        * Else, we verify that the given object is an instance of the class.
-        */
-       if (mptr->common.fn_flags & ZEND_ACC_STATIC) {
-               object = NULL;
-               obj_ce = mptr->common.scope;
-       } else {
-               if (Z_TYPE(params[0]) != IS_OBJECT) {
-                       _DO_THROW("Non-object passed to Invoke()");
-                       /* Returns from this function */
+       if (variadic) {
+               if (zend_parse_parameters(ZEND_NUM_ARGS(), "o!*", &object, &params, &argc) == FAILURE) {
+                       return;
                }
-
-               obj_ce = Z_OBJCE(params[0]);
-
-               if (!instanceof_function(obj_ce, mptr->common.scope)) {
-                       _DO_THROW("Given object is not an instance of the class this method was declared in");
-                       /* Returns from this function */
+       } else {
+               if (zend_parse_parameters(ZEND_NUM_ARGS(), "o!a", &object, &param_array) == FAILURE) {
+                       return;
                }
 
-               object = Z_OBJ(params[0]);
-       }
+               argc = zend_hash_num_elements(Z_ARRVAL_P(param_array));
 
-       fci.size = sizeof(fci);
-       ZVAL_UNDEF(&fci.function_name);
-       fci.object = object;
-       fci.retval = &retval;
-       fci.param_count = num_args - 1;
-       fci.params = params + 1;
-       fci.no_separation = 1;
-
-       fcc.initialized = 1;
-       fcc.function_handler = mptr;
-       fcc.calling_scope = obj_ce;
-       fcc.called_scope = intern->ce;
-       fcc.object = object;
-
-       result = zend_call_function(&fci, &fcc);
-
-       if (result == FAILURE) {
-               zend_throw_exception_ex(reflection_exception_ptr, 0,
-                       "Invocation of method %s::%s() failed", ZSTR_VAL(mptr->common.scope->name), ZSTR_VAL(mptr->common.function_name));
-               return;
-       }
-
-       if (Z_TYPE(retval) != IS_UNDEF) {
-               ZVAL_COPY_VALUE(return_value, &retval);
-       }
-}
-/* }}} */
-
-/* {{{ proto public mixed ReflectionMethod::invokeArgs(mixed object, array args)
-   Invokes the function and pass its arguments as array. */
-ZEND_METHOD(reflection_method, invokeArgs)
-{
-       zval retval;
-       zval *params, *val, *object;
-       reflection_object *intern;
-       zend_function *mptr;
-       int i, argc;
-       int result;
-       zend_fcall_info fci;
-       zend_fcall_info_cache fcc;
-       zend_class_entry *obj_ce;
-       zval *param_array;
-
-       METHOD_NOTSTATIC(reflection_method_ptr);
-
-       GET_REFLECTION_OBJECT_PTR(mptr);
-
-       if (zend_parse_parameters(ZEND_NUM_ARGS(), "o!a", &object, &param_array) == FAILURE) {
-               return;
-       }
-
-       if ((!(mptr->common.fn_flags & ZEND_ACC_PUBLIC)
-                || (mptr->common.fn_flags & ZEND_ACC_ABSTRACT))
-                && intern->ignore_visibility == 0)
-       {
-               if (mptr->common.fn_flags & ZEND_ACC_ABSTRACT) {
-                       zend_throw_exception_ex(reflection_exception_ptr, 0,
-                               "Trying to invoke abstract method %s::%s()",
-                               ZSTR_VAL(mptr->common.scope->name), ZSTR_VAL(mptr->common.function_name));
-               } else {
-                       zend_throw_exception_ex(reflection_exception_ptr, 0,
-                               "Trying to invoke %s method %s::%s() from scope %s",
-                               mptr->common.fn_flags & ZEND_ACC_PROTECTED ? "protected" : "private",
-                               ZSTR_VAL(mptr->common.scope->name), ZSTR_VAL(mptr->common.function_name),
-                               ZSTR_VAL(Z_OBJCE_P(getThis())->name));
-               }
-               return;
+               params = safe_emalloc(sizeof(zval), argc, 0);
+               argc = 0;
+               ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(param_array), val) {
+                       ZVAL_COPY(&params[argc], val);
+                       argc++;
+               } ZEND_HASH_FOREACH_END();
        }
 
-       argc = zend_hash_num_elements(Z_ARRVAL_P(param_array));
-
-       params = safe_emalloc(sizeof(zval), argc, 0);
-       argc = 0;
-       ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(param_array), val) {
-               ZVAL_COPY(&params[argc], val);
-               argc++;
-       } ZEND_HASH_FOREACH_END();
-
        /* In case this is a static method, we should'nt pass an object_ptr
         * (which is used as calling context aka $this). We can thus ignore the
         * first parameter.
@@ -3339,7 +3250,6 @@ ZEND_METHOD(reflection_method, invokeArgs)
                obj_ce = mptr->common.scope;
        } else {
                if (!object) {
-                       efree(params);
                        zend_throw_exception_ex(reflection_exception_ptr, 0,
                                "Trying to invoke non static method %s::%s() without an object",
                                ZSTR_VAL(mptr->common.scope->name), ZSTR_VAL(mptr->common.function_name));
@@ -3349,7 +3259,9 @@ ZEND_METHOD(reflection_method, invokeArgs)
                obj_ce = Z_OBJCE_P(object);
 
                if (!instanceof_function(obj_ce, mptr->common.scope)) {
-                       efree(params);
+                       if (!variadic) {
+                               efree(params);
+                       }
                        _DO_THROW("Given object is not an instance of the class this method was declared in");
                        /* Returns from this function */
                }
@@ -3367,21 +3279,25 @@ ZEND_METHOD(reflection_method, invokeArgs)
        fcc.function_handler = mptr;
        fcc.calling_scope = obj_ce;
        fcc.called_scope = intern->ce;
-       fcc.object = (object) ? Z_OBJ_P(object) : NULL;
+       fcc.object = object ? Z_OBJ_P(object) : NULL;
 
-       /*
-        * Copy the zend_function when calling via handler (e.g. Closure::__invoke())
-        */
-       if ((mptr->internal_function.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) {
-               fcc.function_handler = _copy_function(mptr);
+       if (!variadic) {
+               /*
+                * Copy the zend_function when calling via handler (e.g. Closure::__invoke())
+                */
+               if ((mptr->internal_function.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) {
+                       fcc.function_handler = _copy_function(mptr);
+               }
        }
 
        result = zend_call_function(&fci, &fcc);
 
-       for (i = 0; i < argc; i++) {
-               zval_ptr_dtor(&params[i]);
+       if (!variadic) {
+               for (i = 0; i < argc; i++) {
+                       zval_ptr_dtor(&params[i]);
+               }
+               efree(params);
        }
-       efree(params);
 
        if (result == FAILURE) {
                zend_throw_exception_ex(reflection_exception_ptr, 0,
@@ -3395,6 +3311,22 @@ ZEND_METHOD(reflection_method, invokeArgs)
 }
 /* }}} */
 
+/* {{{ proto public mixed ReflectionMethod::invoke(mixed object, mixed* args)
+   Invokes the method. */
+ZEND_METHOD(reflection_method, invoke)
+{
+       reflection_method_invoke(INTERNAL_FUNCTION_PARAM_PASSTHRU, 1);
+}
+/* }}} */
+
+/* {{{ proto public mixed ReflectionMethod::invokeArgs(mixed object, array args)
+   Invokes the function and pass its arguments as array. */
+ZEND_METHOD(reflection_method, invokeArgs)
+{
+       reflection_method_invoke(INTERNAL_FUNCTION_PARAM_PASSTHRU, 0);
+}
+/* }}} */
+
 /* {{{ proto public bool ReflectionMethod::isFinal()
    Returns whether this method is final */
 ZEND_METHOD(reflection_method, isFinal)

diff --git a/ext/reflection/tests/004.phpt b/ext/reflection/tests/004.phpt
index 41632aa3bab954352be739859216cc44ea8cce28..36ae406b43e8b5589a6b400a085d0ebdce002d60 100644 (file)
--- a/ext/reflection/tests/004.phpt
+++ b/ext/reflection/tests/004.phpt
@@ -38,6 +38,6 @@ try {
 echo "===DONE===\n";?>
 --EXPECTF--
 Deprecated: Methods with the same name as their class will not be constructors in a future version of PHP; a has a deprecated constructor in %s on line %d
-Non-object passed to Invoke()
+Trying to invoke non static method a::a() without an object
 Given object is not an instance of the class this method was declared in
 ===DONE===

diff --git a/ext/reflection/tests/ReflectionMethod_invokeArgs_error3.phpt b/ext/reflection/tests/ReflectionMethod_invokeArgs_error3.phpt
index 41f682cf7f28eebc105d0331c961aae3625666cd..1222467a6ba4d2fdcb6a788ea784c55f34677122 100644 (file)
--- a/ext/reflection/tests/ReflectionMethod_invokeArgs_error3.phpt
+++ b/ext/reflection/tests/ReflectionMethod_invokeArgs_error3.phpt
@@ -115,5 +115,4 @@ string(86) "Trying to invoke private method TestClass::privateMethod() from scop
 
 Abstract method:
 string(53) "Trying to invoke abstract method AbstractClass::foo()"
-
-Warning: ReflectionMethod::invokeArgs() expects exactly 2 parameters, 1 given in %s on line %d
+string(53) "Trying to invoke abstract method AbstractClass::foo()"

diff --git a/ext/reflection/tests/ReflectionMethod_invoke_basic.phpt b/ext/reflection/tests/ReflectionMethod_invoke_basic.phpt
index c0830b277bf55d831bb71c2dd3d84a1e631868bf..7bfe245f821bb82928ceaa567f8130a85310ae74 100644 (file)
--- a/ext/reflection/tests/ReflectionMethod_invoke_basic.phpt
+++ b/ext/reflection/tests/ReflectionMethod_invoke_basic.phpt
@@ -97,8 +97,8 @@ Static method:
 
 Warning: ReflectionMethod::invoke() expects at least 1 parameter, 0 given in %s on line %d
 NULL
-Called staticMethod()
-Exception: Using $this when not in object context
+
+Warning: ReflectionMethod::invoke() expects parameter 1 to be object, boolean given in %s on line %d
 NULL
 Called staticMethod()
 Exception: Using $this when not in object context

diff --git a/ext/reflection/tests/ReflectionMethod_invoke_error1.phpt b/ext/reflection/tests/ReflectionMethod_invoke_error1.phpt
index 758f1acd133be681ffa7fe5daa7cf83fd30301d3..ef5621e7e5284fde7544cf06399e21e0f7987110 100644 (file)
--- a/ext/reflection/tests/ReflectionMethod_invoke_error1.phpt
+++ b/ext/reflection/tests/ReflectionMethod_invoke_error1.phpt
@@ -59,7 +59,9 @@ try {
 ?>
 --EXPECTF--
 invoke() on a non-object:
-string(29) "Non-object passed to Invoke()"
+
+Warning: ReflectionMethod::invoke() expects parameter 1 to be object, boolean given in %s%eReflectionMethod_invoke_error1.php on line %d
+NULL
 
 invoke() on a non-instance:
 string(72) "Given object is not an instance of the class this method was declared in"

diff --git a/ext/reflection/tests/request38992.phpt b/ext/reflection/tests/request38992.phpt
new file mode 100644 (file)
index 0000000..8c0052f
--- /dev/null
+++ b/ext/reflection/tests/request38992.phpt
@@ -0,0 +1,22 @@
+--TEST--
+Request #38992 (invoke() and invokeArgs() static method calls should match)
+--FILE--
+<?php
+class MyClass
+{
+    public static function doSomething()
+    {
+        echo "Did it!\n";
+    }
+}
+
+$r = new ReflectionMethod('MyClass', 'doSomething');
+$r->invoke('WTF?');
+$r->invokeArgs('WTF?', array());
+?>
+===DONE===
+--EXPECTF--
+Warning: ReflectionMethod::invoke() expects parameter 1 to be object, string given in %s%erequest38992.php on line %d
+
+Warning: ReflectionMethod::invokeArgs() expects parameter 1 to be object, string given in %s%erequest38992.php on line %d
+===DONE===