Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95393
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.66 as permitted sender)
References: <CAEbg-4ZAMB=Em-nwCp++K3n59btaFSgW-yexhg3cbM3bQ1WjTg@mail.gmail.com>
 <CAMUwpuQe1bS1D9KVRKkqd7-jXw8sx_mWUDrxDjX=i9J5LQSnwg@mail.gmail.com>
 <dd455ef8-d02f-9e7f-94ba-8e276bce2688@gmx.de>
 <CAFMT4Noh_2BHDUETFtUppJi36m4vuYtmU1A2h_t-Swha1sxKdQ@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Message-ID: <031a2bf1-1996-1144-2b85-e10a40be9514@gmail.com>
Date: Mon, 22 Aug 2016 22:40:11 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: <CAFMT4Noh_2BHDUETFtUppJi36m4vuYtmU1A2h_t-Swha1sxKdQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] BC break: ReflectionMethod::invoke() expects parameter1
 to be object, string given
From: rowan.collins@gmail.com (Rowan Collins)

On 22/08/2016 20:57, Levi Morrison wrote:
> 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]