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

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/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. 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." -- Christoph M. Becker