Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95401
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.17.22 as permitted sender)
To: Julien Pauli <jpauli@php.net>
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>
 <031a2bf1-1996-1144-2b85-e10a40be9514@gmail.com>
 <CAFMT4Nrc7nXx4Q0qZzwUSw2LDsFwe9i9aVNTYOgjeFdhWjEtgg@mail.gmail.com>
 <7b81c830-1d05-a4ac-4713-1bb67cbec12c@gmx.de>
 <CAMUwpuSiLEToYCXzxQnhmtPXi=aO45vAhLWVi-3+U8+TOLYrVg@mail.gmail.com>
Cc: Levi Morrison <levim@php.net>, Rowan Collins <rowan.collins@gmail.com>,
 "internals@lists.php.net" <internals@lists.php.net>
Message-ID: <eb2a9f37-37ea-54fb-e094-fdb8629d159b@gmx.de>
Date: Tue, 23 Aug 2016 13:51:06 +0200
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: <CAMUwpuSiLEToYCXzxQnhmtPXi=aO45vAhLWVi-3+U8+TOLYrVg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] BC break: ReflectionMethod::invoke() expects
 parameter1to be object, string given
From: cmbecker69@gmx.de ("Christoph M. Becker")

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/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."
> 
> 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