# BC break: ReflectionMethod::invoke() expects parameter 1 to be object, string given - Thread: https://externals.io/message/95383 - Original archive: https://news-web.php.net/php.internals/95383 - Started by Alexander Lisachenko on 2016-08-22T15:55:30+00:00 - Messages: 35 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/95383.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #95383 — Alexander Lisachenko — 2016-08-22T15:55:30+00:00 URL: https://externals.io/message/95383#95383 First argument can be a normal string with a valid scope class name for static methods. This can be useful for calling parent static methods, see my example. I think this will be comparable with Closure::call() method and scope binding. понедельник, 22 августа 2016 г. пользователь Rowan Collins написал: > > > > […] > > OK, so that's a further inconsistency. The current version still doesn't > actually make sense, though. If you want to validate that the correct > argument is being passed, then the only value allowed should be null. > > If so desired, I can revert that commit, but I wouldn't be happy with > > […] > > The parameter is still unused, and can be any object. The only sane > validation would be to check explicitly for null, which is documented as > the correct argument for both invoke() and invokeArgs(). > > Regards, > -- > Rowan Collins > [IMSoP] --- ## #95384 — Julien Pauli — 2016-08-22T16:00:18+00:00 URL: https://externals.io/message/95383#95384 · In reply to: #95383 I agree this is a BC break and should not stay as-is in source code. It makes some testsuites fail, that did not fail before ; thus it breaks things. I think @Alexander is right. We should allow passing a string, and perform some Late Static Binding through it. That would solve the BC Break problem as well as extend the feature in a more PHP-friendly way. Julien On Mon, Aug 22, 2016 at 5:55 PM, Alexander Lisachenko wrote: > First argument can be a normal string with a valid scope class name for > static methods. This can be useful for calling parent static methods, see > my example. > > I think this will be comparable with Closure::call() method and scope > binding. > > понедельник, 22 августа 2016 г. пользователь Rowan Collins написал: > > > […] --- ## #95385 — Christoph M. Becker — 2016-08-22T16:30:24+00:00 URL: https://externals.io/message/95383#95385 · In reply to: #95384 > I agree this is a BC break and should not stay as-is in source code. I wonder why we have more than 100 lines of "Backward incompatible changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be introduced in a minor release. > It makes some testsuites fail, that did not fail before ; thus it breaks things. An estimated 10% (at least) of my *bugfixes* in GD broke at least one PHPT, because the test was broken in the first place. > I think @Alexander is right. > We should allow passing a string, and perform some Late Static Binding > through it. > That would solve the BC Break problem as well as extend the feature in > a more PHP-friendly way. Are you sure that would solve the BC break? As it were, one could pass an arbitrary string. I can easily imagine that some refactoring introduced a new class in a hierarchy, but nobody noticed that an ::invoke() call would have to be adjusted accordingly. Suddenly changing the meaning of the first parameter may well introduce a behavioral change. [1] -- Christoph M. Becker --- ## #95386 — Julien Pauli — 2016-08-22T16:34:20+00:00 URL: https://externals.io/message/95383#95386 · In reply to: #95385 > > > > […] > > I wonder why we have more than 100 lines of "Backward incompatible > changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be > introduced in a minor release. > > > […] > > An estimated 10% (at least) of my *bugfixes* in GD broke at least one > PHPT, because the test was broken in the first place. > > > […] > > Are you sure that would solve the BC break? As it were, one could pass > an arbitrary string. I can easily imagine that some refactoring > introduced a new class in a hierarchy, but nobody noticed that an > ::invoke() call would have to be adjusted accordingly. Suddenly > changing the meaning of the first parameter may well introduce a > behavioral change. Well, we must find a solution that tries to cope with everyone needs, and tries to make PHP more consistent if possible. I know it is not an easy topic , but I'm sure we'll find some consensus. I'll prepare a patch exposing my ideas soon. Julien --- ## #95389 — Rowan Collins — 2016-08-22T18:34:14+00:00 URL: https://externals.io/message/95383#95389 · In reply to: #95385 > > […] > Are you sure that would solve the BC break? As it were, one could pass > an arbitrary string. I can easily imagine that some refactoring > introduced a new class in a hierarchy, but nobody noticed that an > ::invoke() call would have to be adjusted accordingly. Suddenly > changing the meaning of the first parameter may well introduce a > behavioral change. Yeah, that was basically my concern as well: giving the string parameter a meaning is no more compatible than rejecting it. It is, at least, more *useful*, but it does make the same code do subtly different things in the new version. One question which would need addressing is what should happen if you reflect a static method call, and pass in an object as the invocation context: - As currently, even with the patch that's in beta, ignore the object completely? (This seems broken to me.) - Reject it outright, saying that it must be a string or null? (Simple, but not very user-friendly.) - Verify that it's of the right type (as with a non-static invocation), then take its class as the context of the call? (Seems best, but possibly complex?) Altogether, I think this may need more thought than a rushed fix during a beta cycle. How about reverting the behaviour for 7.1 and changing carefully in 7.2? Regards, -- Rowan Collins [IMSoP] --- ## #95390 — Levi Morrison — 2016-08-22T19:57:51+00:00 URL: https://externals.io/message/95383#95390 · In reply to: #95385 On Mon, Aug 22, 2016 at 10:30 AM, Christoph M. Becker wrote: > > > > […] > > I wonder why we have more than 100 lines of "Backward incompatible > changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be > introduced in a minor release. > > > […] > things. > > An estimated 10% (at least) of my *bugfixes* in GD broke at least one > PHPT, because the test was broken in the first place. > > > […] > > Are you sure that would solve the BC break? As it were, one could pass > an arbitrary string. I can easily imagine that some refactoring > introduced a new class in a hierarchy, but nobody noticed that an > ::invoke() call would have to be adjusted accordingly. Suddenly > changing the meaning of the first parameter may well introduce a > behavioral change. > > [1] > […] I was a little unclear in the beginning, so let me clear something up now: if it's not documented how can anyone be sure what the correct behavior is? Attempting to preserve backwards compatibility on something undefined is meaningless. Now if something was simply not documented that's a bit different. I think the ideal but difficult path forward is to find where it was introduced and pray the commit log or something else will indicate the behavior. Only then can we properly preserve it. Until then it is undefined behavior that I don't care one whit about. And attempting to "improve it" by adding late static binding somehow would also be pointless until the original behavior is identified. As it stands I am fine to keep the BC break or revert it - doesn't really matter. But if anyone *does* care then they need to do some digging. --- ## #95393 — Rowan Collins — 2016-08-22T21:40:11+00:00 URL: https://externals.io/message/95383#95393 · In reply to: #95390 > I think the ideal but difficult path forward is to find where it was > introduced and pray the commit log or something else will indicate the > behavior. OK, I'll bite. Christoph already linked to this comment in the source [https://github.com/php/php-src/blob/PHP-7.0.10/ext/reflection/php_reflection.c#L3197-L3202]: > /* 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. > */ A simple blame takes that comment back effectively unchanged to Nov 2005, when reflection was first moved to "ext/reflection": https://github.com/php/php-src/blob/7cb0480d04933e3d27b75edf29822815a6108894/ext/reflection/php_reflection.c#L2163 Before that, it was in zend_reflection_api.c, and blames back to the rather general "more of Timm's implementation" committed by George Schlossnagle in July 2003: https://github.com/php/php-src/commit/84f5e4870e13f76a6223a0a937809092ae70d543#diff-cf9733a6fe0eeed1f5a44b59667967baR984 The inconsistency then comes in when invokeArgs is added by Marcus Boerger over a year later: https://github.com/php/php-src/commit/41b87ab486c26f9b2d1bc315988b8e8271b6e06b The new methods made use of the (presumably new?) zend_parse_parameters system, and specified the first argument as a mandatory object, which was fixed a few days later to have an optional object, and a separate check when it is required: https://github.com/php/php-src/commit/63b288c4646d405d0edfb7657505b2acf5643514 Notably, the same comment completely ignoring the first parameter was present in that first implementation of invokeArgs. So, it's pretty clear to me that there was no intention for the two to be different, just different contributors at different times. It's also pretty clear that the only thought put into the first argument with static methods is "ignore it". None of which really answers what the behaviour should be, in my opinion. We still have to decide 3 things: - Is there a compelling reason to change the current behaviour? - What error or behaviour should a string or other non-object argument give? - What error or behaviour should an object argument give? In my opinion, the best "fix", if something needs to change, would be to reject anything other than null; that anything else works appears to just be an oversight. Regards, -- Rowan Collins [IMSoP] --- ## #95394 — Levi Morrison — 2016-08-22T22:25:57+00:00 URL: https://externals.io/message/95383#95394 · In reply to: #95393 > > > > […] > > > OK, I'll bite. > > Christoph already linked to this comment in the source [https://github.com/php/php-src/blob/PHP-7.0.10/ext/reflection/php_reflection.c#L3197-L3202]: > > > […] > > > A simple blame takes that comment back effectively unchanged to Nov 2005, when reflection was first moved to "ext/reflection": > https://github.com/php/php-src/blob/7cb0480d04933e3d27b75edf29822815a6108894/ext/reflection/php_reflection.c#L2163 > > Before that, it was in zend_reflection_api.c, and blames back to the rather general "more of Timm's implementation" committed by George Schlossnagle in July 2003: https://github.com/php/php-src/commit/84f5e4870e13f76a6223a0a937809092ae70d543#diff-cf9733a6fe0eeed1f5a44b59667967baR984 > > The inconsistency then comes in when invokeArgs is added by Marcus Boerger over a year later: https://github.com/php/php-src/commit/41b87ab486c26f9b2d1bc315988b8e8271b6e06b > > The new methods made use of the (presumably new?) zend_parse_parameters system, and specified the first argument as a mandatory object, which was fixed a few days later to have an optional object, and a separate check when it is required: https://github.com/php/php-src/commit/63b288c4646d405d0edfb7657505b2acf5643514 > > Notably, the same comment completely ignoring the first parameter was present in that first implementation of invokeArgs. > > So, it's pretty clear to me that there was no intention for the two to be different, just different contributors at different times. It's also pretty clear that the only thought put into the first argument with static methods is "ignore it". > > None of which really answers what the behaviour should be, in my opinion. We still have to decide 3 things: > > - Is there a compelling reason to change the current behaviour? > - What error or behaviour should a string or other non-object argument give? > - What error or behaviour should an object argument give? > > In my opinion, the best "fix", if something needs to change, would be to reject anything other than null; that anything else works appears to just be an oversight. > > Regards, > > -- > Rowan Collins > [IMSoP] > > > -- > > Anyone oppose to emitting `E_DEPRECATED` for a parameter other than an object or null? This opens up the possibility to use it for something no earlier than 8.0. --- ## #95397 — Pierre Joye — 2016-08-23T00:43:52+00:00 URL: https://externals.io/message/95383#95397 · In reply to: #95394 > > Anyone oppose to emitting `E_DEPRECATED` for a parameter other than an > object or null? This opens up the possibility to use it for something > no earlier than 8.0. I won't oppose it and sounds reasonable. I still prefer Julien's proposal, let see when we have the patch :) Cheers Pierre --- ## #95398 — Christoph M. Becker — 2016-08-23T08:30:39+00:00 URL: https://externals.io/message/95383#95398 · In reply to: #95394 > > > […] Thanks for investigating on the history, Rowan! > > […] ACK > > […] > > Anyone oppose to emitting `E_DEPRECATED` for a parameter other than an > object or null? This opens up the possibility to use it for something > no earlier than 8.0. That appears to be the most reasonable compromise presented yet. Thanks Levi! I suggest to wait for Julian, though, who wrote: "I'll prepare a patch exposing my ideas soon." -- Christoph M. Becker --- ## #95399 — lauri.kentta@gmail.com — 2016-08-23T09:16:53+00:00 URL: https://externals.io/message/95383#95399 · In reply to: #95398 > > > > […] > > That appears to be the most reasonable compromise presented yet. > Thanks > Levi! I suggest to wait for Julian, though, who wrote: "I'll prepare a > patch exposing my ideas soon." Would there be different checks for static and non-static? - For non-static, accept object or null. (Or only a non-null object?) - For static, accept only null (deprecate also objects). -- Lauri Kenttä --- ## #95400 — Julien Pauli — 2016-08-23T11:36:49+00:00 URL: https://externals.io/message/95383#95400 · In reply to: #95398 > > > > […] > > Thanks for investigating on the history, Rowan! > > > […] > > ACK > > > […] > > That appears to be the most reasonable compromise presented yet. Thanks > Levi! I suggest to wait for Julian, though, who wrote: "I'll prepare a > patch exposing my ideas soon." All right. I was thinking something like this : https://github.com/jpauli/php-src/commit/8735d6b72de0edc3b370245737c0f4f204fcbea0 But it makes no sense. It makes no sense to pass a class name as first param of invoke() , to target a specific function. Why not create the ReflectionClass instance using that latter so ? I'm OK to deprecate the first arg as a string, then remove such an usage in next major. I can make such a patch if consensus. Julien --- ## #95401 — Christoph M. Becker — 2016-08-23T11:51:06+00:00 URL: https://externals.io/message/95383#95401 · In reply to: #95400 > > > > […] > > All right. > > I was thinking something like this : > https://github.com/jpauli/php-src/commit/8735d6b72de0edc3b370245737c0f4f204fcbea0 > > But it makes no sense. > > It makes no sense to pass a class name as first param of invoke() , to > target a specific function. > Why not create the ReflectionClass instance using that latter so ? > > I'm OK to deprecate the first arg as a string, then remove such an > usage in next major. > > I can make such a patch if consensus. I suggest to deprecate all other types than `NULL` as first arg for static methods, because passing an int, for instance, makes even less sense as Rowan has already pointed out elsewhere in this thread. -- Christoph M. Becker --- ## #95403 — Julien Pauli — 2016-08-23T12:51:17+00:00 URL: https://externals.io/message/95383#95403 · In reply to: #95401 > > > > […] > > I suggest to deprecate all other types than `NULL` as first arg for static > methods, because passing an int, for instance, makes even less sense as > Rowan has already pointed out elsewhere in this thread. What about passing nothing ? $reflectionMeth->invoke(); Actually, this is not possible and a Warning: ReflectionMethod::invoke() expects at least 1 parameter, 0 given in %s on line %d is thrown. Should we keep that behavior as well ? Knowing that passing nothing, the function will get an IS_NULL zval. Julien --- ## #95404 — Alexander Lisachenko — 2016-08-23T12:56:41+00:00 URL: https://externals.io/message/95383#95404 · In reply to: #95401 2016-08-23 14:51 GMT+03:00 Christoph M. Becker : > I suggest to deprecate all other types than `NULL` as first arg for static > methods, because passing an int, for instance, makes even less sense as > Rowan has already pointed out elsewhere in this thread. > Alternative suggestion (vote for Julien's patch): use first argument as a string with LSB scope name, see my message from late 2013 year: http://markmail.org/message/ogeh33jedumgo5lx. I agree, that all other types don't make any sense. So my suggestion is following: for dynamic methods it should accept object or null. Null can mean that we want to unbind the method from the object and maybe call it like a closure? (just compare this with Closure::bindTo behaviour). For static methods this could be object (why not - it will be ignored), string (in this case it will be a scope name if we want to call parent method with preserving Late Static Binding) or just null (null will be equal to the string with object class name) Thoughts? --- ## #95405 — Julien Pauli — 2016-08-23T13:01:57+00:00 URL: https://externals.io/message/95383#95405 · In reply to: #95404 On Tue, Aug 23, 2016 at 2:56 PM, Alexander Lisachenko wrote: > > 2016-08-23 14:51 GMT+03:00 Christoph M. Becker : > > […] > > > > Alternative suggestion (vote for Julien's patch): use first argument as a > string with LSB scope name, see my message from late 2013 year: > http://markmail.org/message/ogeh33jedumgo5lx. > > I agree, that all other types don't make any sense. So my suggestion is > following: for dynamic methods it should accept object or null. Null can > mean that we want to unbind the method from the object and maybe call it > like a closure? (just compare this with Closure::bindTo behaviour). For > static methods this could be object (why not - it will be ignored), string > (in this case it will be a scope name if we want to call parent method with > preserving Late Static Binding) or just null (null will be equal to the > string with object class name) > > Thoughts? > It makes no sense to change the scope of the method. A method is not a closure. A method is bound to a class , a closure is by default unbound. A method cannot be unbound. A method cannot change from one class to another. This : class A { public static function foo() { } } class B extends A { public static function foo() { } } $a = new reflectionMethod('A', 'foo'); $a->invoke('B'); would lead to the exact equivalent as this with my patch : $b = new reflectionMethod('B', 'foo'); $b->invoke(); So I don't see why we should change and patch the code to support such a strange use case ... If you want to call B's foo , create a ReflectionMethod from B, not from A. Julien --- ## #95406 — Alexander Lisachenko — 2016-08-23T13:07:38+00:00 URL: https://externals.io/message/95383#95406 · In reply to: #95405 2016-08-23 16:01 GMT+03:00 Julien Pauli : > This : > > class A { public static function foo() { } } > class B extends A { public static function foo() { } } > > $a = new reflectionMethod('A', 'foo'); > $a->invoke('B'); > It's perfect way for me. Just to be sure: in this case we will invoke A::foo() method, but the scope (static::class) will be 'B', right? If yes, then everything is ok and it will be compatible with all existing code but will have an additional meaning for calling methods with children's scope. --- ## #95408 — Rowan Collins — 2016-08-23T13:11:25+00:00 URL: https://externals.io/message/95383#95408 · In reply to: #95404 > For static methods this could be object (why not - it will be ignored) Allowing a parameter and then completely ignoring it is what got us into this situation in the first place. Why does this... $a = new reflectionMethod('A', 'foo'); $a->invoke(new DateTime); ...make any more sense than this? $a = new reflectionMethod('A', 'foo'); $a->invoke(42); If you want to allow an object to be passed in, it should act the same way as a string - specify the binding context: $a = new reflectionMethod('A', 'foo'); $a->invoke(new B); // equivalent to $a->invoke('B'); It should also go through the same check as it would for a non-static method: must be an instance of the class being reflected. Regards, -- Rowan Collins [IMSoP] --- ## #95407 — Julien Pauli — 2016-08-23T13:13:14+00:00 URL: https://externals.io/message/95383#95407 · In reply to: #95406 On Tue, Aug 23, 2016 at 3:07 PM, Alexander Lisachenko wrote: > > 2016-08-23 16:01 GMT+03:00 Julien Pauli : > > […] > > > > It's perfect way for me. > Just to be sure: in this case we will invoke A::foo() method, but the scope > (static::class) will be 'B', right? If yes, then everything is ok and it No, How can this be ? B extends A , not A extends B ... In this case, B'sfoo() will be called , which is the equivalent to having a reflectionMethod from B , so useless to me. Julien --- ## #95409 — Alexander Lisachenko — 2016-08-23T13:20:31+00:00 URL: https://externals.io/message/95383#95409 · In reply to: #95407 2016-08-23 16:13 GMT+03:00 Julien Pauli : > > No, How can this be ? B extends A , not A extends B ... > In this case, B'sfoo() will be called , which is the equivalent to > having a reflectionMethod from B , so useless to me. Ok, how can I invoke Parent::whoami() with reflection and get exactly "Child" as an output? class Parent { public static function whoami() { echo static::class; } } class Child extends Parent { public static function whoami() { echo "Don't call me now!'; parent::whoami(); } } --- ## #95410 — Rowan Collins — 2016-08-23T13:37:26+00:00 URL: https://externals.io/message/95383#95410 · In reply to: #95409 > > 2016-08-23 16:13 GMT+03:00 Julien Pauli >: > > > No, How can this be ? B extends A , not A extends B ... > In this case, B'sfoo() will be called , which is the equivalent to > having a reflectionMethod from B , so useless to me. > > > > Ok, how can I invoke Parent::whoami() with reflection and get exactly > "Child" as an output? I've no idea why you'd want to, but note that using get_class($this), you can actually get that for a non-static method: https://3v4l.org/Rc7Dk class MyParent { public function whoami() { echo get_class($this); } } class MyChild extends MyParent { public function whoami() { echo "Don't call me now!"; parent::whoami(); } } $m = new reflectionMethod('MyParent', 'whoami'); $m->invoke(new MyChild); So there is a gap in functionality between static and non-static methods here. I'm not sure reinterpreting a parameter which has always been ignored is the best way to solve it though. Regards, -- Rowan Collins [IMSoP] --- ## #95411 — Julien Pauli — 2016-08-23T13:40:18+00:00 URL: https://externals.io/message/95383#95411 · In reply to: #95409 On Tue, Aug 23, 2016 at 3:20 PM, Alexander Lisachenko wrote: > > 2016-08-23 16:13 GMT+03:00 Julien Pauli : > > […] > > > > Ok, how can I invoke Parent::whoami() with reflection and get exactly > "Child" as an output? > > class Parent { > public static function whoami() { > echo static::class; > } > } > > class Child extends Parent { > public static function whoami() { > echo "Don't call me now!'; > parent::whoami(); > } > } > My patch allows that, but I can't find a use case to it. Try it. Julien --- ## #95412 — Alexander Lisachenko — 2016-08-23T13:56:47+00:00 URL: https://externals.io/message/95383#95412 · In reply to: #95411 2016-08-23 16:40 GMT+03:00 Julien Pauli : > My patch allows that, but I can't find a use case to it. My use case for that was decorating of static methods with additional behaviour, so it's possible to cache the result of static methods, etc. Unfortunately, reflection API doesn't provide me such an ability, so I decided to switch to the combination of special closure with `forward_static_call_array()` and binding it to the desired scope. 2016-08-23 16:40 GMT+03:00 Julien Pauli : > Try it. Yes, sure, I will check. However as I say earlier, now I use closure extraction and scope binding as more natural way to invoke such methods. So missing scope argument for static methods and ReflectionMethod->invoke() is a nice thing to have, but not a critical one. --- ## #95413 — Levi Morrison — 2016-08-23T14:13:07+00:00 URL: https://externals.io/message/95383#95413 · In reply to: #95412 On Tue, Aug 23, 2016 at 7:56 AM, Alexander Lisachenko wrote: > > 2016-08-23 16:40 GMT+03:00 Julien Pauli : > > […] > > > > My use case for that was decorating of static methods with additional > behaviour, so it's possible to cache the result of static methods, etc. > Unfortunately, reflection API doesn't provide me such an ability, so I > decided to switch to the combination of special closure with > `forward_static_call_array()` and binding it to the desired scope. > > > 2016-08-23 16:40 GMT+03:00 Julien Pauli : > > […] > > > Yes, sure, I will check. However as I say earlier, now I use closure > extraction and scope binding as more natural way to invoke such methods. So > missing scope argument for static methods and ReflectionMethod->invoke() is > a nice thing to have, but not a critical one. Seems to me you should be calling getClosure() and using Closure::bindTo before invoking it. ReflectionMethod::invoke is really just a short-cut for a common case; I don't think it's reasonable to have it mirror the Closure API just to save one method call to get a Closure. --- ## #95414 — Julien Pauli — 2016-08-23T14:49:54+00:00 URL: https://externals.io/message/95383#95414 · In reply to: #95413 > On Tue, Aug 23, 2016 at 7:56 AM, Alexander Lisachenko > wrote: > > […] > > Seems to me you should be calling getClosure() and using > Closure::bindTo before invoking it. ReflectionMethod::invoke is really > just a short-cut for a common case; I don't think it's reasonable to > have it mirror the Closure API just to save one method call to get a > Closure. This cannot work. There is a special use case forbidding that. "Cannot rebind scope of closure created by ReflectionFunctionAbstract::getClosure()" Julien --- Next page: https://externals.io/message/95383.md?page=2