Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95400
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)
MIME-Version: 1.0
Sender: julienpauli@gmail.com
In-Reply-To: <7b81c830-1d05-a4ac-4713-1bb67cbec12c@gmx.de>
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>
Date: Tue, 23 Aug 2016 13:36:49 +0200
Message-ID: <CAMUwpuSiLEToYCXzxQnhmtPXi=aO45vAhLWVi-3+U8+TOLYrVg@mail.gmail.com>
To: "Christoph M. Becker" <cmbecker69@gmx.de>
Cc: Levi Morrison <levim@php.net>, Rowan Collins <rowan.collins@gmail.com>, 
	"internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] BC break: ReflectionMethod::invoke() expects
 parameter1to be object, string given
From: jpauli@php.net (Julien Pauli)

On Tue, Aug 23, 2016 at 10:30 AM, Christoph M. Becker <cmbecker69@gmx.de> w=
rote:
> 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 ignor=
e the
>>>>      * first parameter.
>>>>      *
>>>>      * Else, we verify that the given object is an instance of the cla=
ss..
>>>>      */
>>>
>>> A simple blame takes that comment back effectively unchanged to Nov 200=
5, when reflection was first moved to "ext/reflection":
>>> https://github.com/php/php-src/blob/7cb0480d04933e3d27b75edf29822815a61=
08894/ext/reflection/php_reflection.c#L2163
>>>
>>> Before that, it was in zend_reflection_api.c, and blames back to the ra=
ther general "more of Timm's implementation" committed by George Schlossnag=
le in July 2003: https://github.com/php/php-src/commit/84f5e4870e13f76a6223=
a0a937809092ae70d543#diff-cf9733a6fe0eeed1f5a44b59667967baR984
>>>
>>> The inconsistency then comes in when invokeArgs is added by Marcus Boer=
ger over a year later: https://github.com/php/php-src/commit/41b87ab486c26f=
9b2d1bc315988b8e8271b6e06b
>>>
>>> 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 whe=
n it is required: https://github.com/php/php-src/commit/63b288c4646d405d0ed=
fb7657505b2acf5643514
>>>
>>> Notably, the same comment completely ignoring the first parameter was p=
resent 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 pre=
tty clear that the only thought put into the first argument with static met=
hods is "ignore it".
>
> ACK
>
>>> None of which really answers what the behaviour should be, in my opinio=
n. 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 t=
o 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/8735d6b72de0edc3b370245737c0f4f204=
fcbea0

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