Re: BC break: ReflectionMethod::invoke() expects parameter1to be object, string given

On Tue, Aug 23, 2016 at 1:51 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote: > On 23.08.2016 at 13:36, Julien Pauli wrote: > >> On Tue, Aug 23, 2016 at 10:30 AM, Christoph M. Becker <cmbecker69@gmx.de> wrote: >> >>> On 23.08.2016 at 00:25, Levi Morrison wrote: >>> >>>> On Mon, Aug 22, 2016 at 3:40 PM, Rowan Collins <rowan.collins@gmail.com> >>>> wrote: >>>>> >>>>> 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/41b87ab486c26f9b2d1bc315988b8e8271b6e06bY��c��V�s� >>>>> ��� >>>>> >>>>> 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. >>> >>> Thanks for investigating on the history, Rowan! >>> >>>>> 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". >>> >>> ACK >>> >>>>> 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. >>>> >>>> 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." >> >> 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. 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