Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Re: BC break: ReflectionMethod::invoke() expects parameter 1 to be object, string given »

Re: BC break: ReflectionMethod::invoke() expects parameter 1 to be object, string given

php.internals

Alexander Lissachenko

10 years ago
First argument can be a normal string with a valid scope class name for static methods. This can be useful for calling parent static methods, see my example. I think this will be comparable with Closure::call() method and scope binding. понедельник, 22 августа 2016 г. пользователь Rowan Collins написал:

Julien Pauli

10 years ago
I agree this is a BC break and should not stay as-is in source code. It makes some testsuites fail, that did not fail before ; thus it breaks things. I think @Alexander is right. We should allow passing a string, and perform some Late Static Binding through it. That would solve the BC Break problem as well as extend the feature in a more PHP-friendly way. Julien On Mon, Aug 22, 2016 at 5:55 PM, Alexander Lisachenko <lisachenko.it@gmail.com> wrote:

Christoph Becker

10 years ago
On 22.08.2016 at 18:00, Julien Pauli wrote:
> I agree this is a BC break and should not stay as-is in source code.
I wonder why we have more than 100 lines of "Backward incompatible changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be introduced in a minor release.
> It makes some testsuites fail, that did not fail before ; thus it breaks things.
An estimated 10% (at least) of my *bugfixes* in GD broke at least one PHPT, because the test was broken in the first place.
> I think @Alexander is right. > We should allow passing a string, and perform some Late Static Binding > through it. > That would solve the BC Break problem as well as extend the feature in > a more PHP-friendly way.
Are you sure that would solve the BC break? As it were, one could pass an arbitrary string. I can easily imagine that some refactoring introduced a new class in a hierarchy, but nobody noticed that an ::invoke() call would have to be adjusted accordingly. Suddenly changing the meaning of the first parameter may well introduce a behavioral change. [1] <https://github.com/php/php-src/blob/PHP-7.1.0beta3/UPGRADING#L18-L129>
-- Christoph M. Becker

Julien Pauli

10 years ago
On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 22.08.2016 at 18:00, Julien Pauli wrote: > >> I agree this is a BC break and should not stay as-is in source code. > > I wonder why we have more than 100 lines of "Backward incompatible > changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be > introduced in a minor release. > >> It makes some testsuites fail, that did not fail before ; thus it breaks things. > > An estimated 10% (at least) of my *bugfixes* in GD broke at least one > PHPT, because the test was broken in the first place. > >> I think @Alexander is right. >> We should allow passing a string, and perform some Late Static Binding >> through it. >> That would solve the BC Break problem as well as extend the feature in >> a more PHP-friendly way. > > Are you sure that would solve the BC break? As it were, one could pass > an arbitrary string. I can easily imagine that some refactoring > introduced a new class in a hierarchy, but nobody noticed that an > ::invoke() call would have to be adjusted accordingly. Suddenly > changing the meaning of the first parameter may well introduce a > behavioral change.
Well, we must find a solution that tries to cope with everyone needs, and tries to make PHP more consistent if possible. I know it is not an easy topic , but I'm sure we'll find some consensus. I'll prepare a patch exposing my ideas soon. Julien

Rowan Collins

10 years ago
On 22/08/2016 17:30, Christoph M. Becker wrote:
>> I think @Alexander is right. >> >We should allow passing a string, and perform some Late Static Binding >> >through it. >> >That would solve the BC Break problem as well as extend the feature in >> >a more PHP-friendly way. > Are you sure that would solve the BC break? As it were, one could pass > an arbitrary string. I can easily imagine that some refactoring > introduced a new class in a hierarchy, but nobody noticed that an > ::invoke() call would have to be adjusted accordingly. Suddenly > changing the meaning of the first parameter may well introduce a > behavioral change.
Yeah, that was basically my concern as well: giving the string parameter a meaning is no more compatible than rejecting it. It is, at least, more *useful*, but it does make the same code do subtly different things in the new version. One question which would need addressing is what should happen if you reflect a static method call, and pass in an object as the invocation context: - As currently, even with the patch that's in beta, ignore the object completely? (This seems broken to me.) - Reject it outright, saying that it must be a string or null? (Simple, but not very user-friendly.) - Verify that it's of the right type (as with a non-static invocation), then take its class as the context of the call? (Seems best, but possibly complex?) Altogether, I think this may need more thought than a rushed fix during a beta cycle. How about reverting the behaviour for 7.1 and changing carefully in 7.2? Regards,
-- Rowan Collins [IMSoP]

Levi Morrison

10 years ago
On Mon, Aug 22, 2016 at 10:30 AM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 22.08.2016 at 18:00, Julien Pauli wrote: > > > I agree this is a BC break and should not stay as-is in source code. > > I wonder why we have more than 100 lines of "Backward incompatible > changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be > introduced in a minor release. > > > It makes some testsuites fail, that did not fail before ; thus it breaks > things. > > An estimated 10% (at least) of my *bugfixes* in GD broke at least one > PHPT, because the test was broken in the first place. > > > I think @Alexander is right. > > We should allow passing a string, and perform some Late Static Binding > > through it. > > That would solve the BC Break problem as well as extend the feature in > > a more PHP-friendly way. > > Are you sure that would solve the BC break? As it were, one could pass > an arbitrary string. I can easily imagine that some refactoring > introduced a new class in a hierarchy, but nobody noticed that an > ::invoke() call would have to be adjusted accordingly. Suddenly > changing the meaning of the first parameter may well introduce a > behavioral change. > > [1] <https://github.com/php/php-src/blob/PHP-7.1.0beta3/UPGRADING#L18-L129 > >
I was a little unclear in the beginning, so let me clear something up now: if it's not documented how can anyone be sure what the correct behavior is? Attempting to preserve backwards compatibility on something undefined is meaningless. Now if something was simply not documented that's a bit different. 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. Only then can we properly preserve it. Until then it is undefined behavior that I don't care one whit about. And attempting to "improve it" by adding late static binding somehow would also be pointless until the original behavior is identified. As it stands I am fine to keep the BC break or revert it - doesn't really matter. But if anyone *does* care then they need to do some digging.

Rowan Collins

10 years ago
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]

Levi Morrison

10 years ago
On Mon, Aug 22, 2016 at 3:40 PM, Rowan Collins <rowan.collins@gmail.com> wrote:
> > 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] > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php
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.

Pierre Joye

10 years ago
On Aug 23, 2016 5:26 AM, "Levi Morrison" <levim@php.net> wrote:
>
> 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.
I won't oppose it and sounds reasonable. I still prefer Julien's proposal, let see when we have the patch :) Cheers Pierre

Christoph Becker

10 years ago
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

Lauri Kenttä

10 years ago
On 2016-08-23 11:30, Christoph M. Becker 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: >>> >>> 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."
Would there be different checks for static and non-static? - For non-static, accept object or null. (Or only a non-null object?) - For static, accept only null (deprecate also objects).
-- Lauri Kenttä

Julien Pauli

10 years ago
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. Julien

Christoph Becker

10 years ago
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

Julien Pauli

10 years ago
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/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.
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

Alexander Lissachenko

10 years ago
2016-08-23 14:51 GMT+03:00 Christoph M. Becker <cmbecker69@gmx.de>:
> 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. >
Alternative suggestion (vote for Julien's patch): use first argument as a string with LSB scope name, see my message from late 2013 year: http://markmail.org/message/ogeh33jedumgo5lx. I agree, that all other types don't make any sense. So my suggestion is following: for dynamic methods it should accept object or null. Null can mean that we want to unbind the method from the object and maybe call it like a closure? (just compare this with Closure::bindTo behaviour). For static methods this could be object (why not - it will be ignored), string (in this case it will be a scope name if we want to call parent method with preserving Late Static Binding) or just null (null will be equal to the string with object class name) Thoughts?

Ferenc Kovacs

10 years ago
On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 22.08.2016 at 18:00, Julien Pauli wrote: > > > I agree this is a BC break and should not stay as-is in source code. > > I wonder why we have more than 100 lines of "Backward incompatible > changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be > introduced in a minor release. >
That's a bit loaded question, and leads to a broken windows situation, but from my understanding some people read https://wiki.php.net/rfc/releaseprocess differently: consider some BC breaks simply bug fixes, or think that we shouldn't stick to absolutes but consider BC breaks on a case-by-case basis. personally I think tha
> > > It makes some testsuites fail, that did not fail before ; thus it breaks > things. > > An estimated 10% (at least) of my *bugfixes* in GD broke at least one > PHPT, because the test was broken in the first place. >
test failures can be false positive or depending explicitly undefined behaviors, but they can be a good indicator when looking for BC breaks. as we can see from the previous mails in this thread there are behavior changes where the previous behavior was different from what was documented so a bit of a grey area. personally I think that we are in general too lenient with allowing BC breaks in 7.1 (even though that I somehow expected this and was arguing for a longer release cycle for 7.0 or at least having a clear roadmap for the next major version) and we should be more strict about it otherwise we will lose the trust we gained from the userland in the last couple of years with our release process and versioning.
-- Ferenc Kovács @Tyr43l - http://tyrael.hu

Christoph Becker

10 years ago
On 23.08.2016 at 17:03, Ferenc Kovacs wrote:
> personally I think that we are in general too lenient with allowing BC > breaks in 7.1 (even though that I somehow expected this and was arguing for > a longer release cycle for 7.0 or at least having a clear roadmap for the > next major version) and we should be more strict about it otherwise we will > lose the trust we gained from the userland in the last couple of years with > our release process and versioning.
I agree that we should have a plan for the roadmap to the next major release. Otherwise many might be afraid that it takes ten years again, and therefore might be more inclined to accept BC breaks in a minor version ("better a BC now, than to wait too long for the improvement").
-- Christoph M. Becker

Levi Morrison

10 years ago
On Tue, Aug 23, 2016 at 9:03 AM, Ferenc Kovacs <tyra3l@gmail.com> wrote:
> On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker <cmbecker69@gmx.de> > wrote: > >> On 22.08.2016 at 18:00, Julien Pauli wrote: >> >> > I agree this is a BC break and should not stay as-is in source code. >> >> I wonder why we have more than 100 lines of "Backward incompatible >> changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be >> introduced in a minor release. >> > > That's a bit loaded question, and leads to a broken windows situation, but > from my understanding some people read > https://wiki.php.net/rfc/releaseprocess differently: consider some BC > breaks simply bug fixes, or think that we shouldn't stick to absolutes but > consider BC breaks on a case-by-case basis. > personally I think tha > > >> >> > It makes some testsuites fail, that did not fail before ; thus it breaks >> things. >> >> An estimated 10% (at least) of my *bugfixes* in GD broke at least one >> PHPT, because the test was broken in the first place. >> > > test failures can be false positive or depending explicitly undefined > behaviors, but they can be a good indicator when looking for BC breaks. > as we can see from the previous mails in this thread there are behavior > changes where the previous behavior was different from what was documented > so a bit of a grey area. > > personally I think that we are in general too lenient with allowing BC > breaks in 7.1 (even though that I somehow expected this and was arguing for > a longer release cycle for 7.0 or at least having a clear roadmap for the > next major version) and we should be more strict about it otherwise we will > lose the trust we gained from the userland in the last couple of years with > our release process and versioning. > > > > -- > Ferenc Kovács > @Tyr43l - http://tyrael.hu
I agree. However in this particular instance emitting an E_DEPRECATED like I proposed should be helpful. These people who are passing strings are not getting any behavior out of it - its' completely ignored. Additionally since its an E_DEPRECATED it's highly unlikely to actually break something. What do you think?

Julien Pauli

10 years ago
On Thu, Aug 25, 2016 at 9:48 PM, Levi Morrison <levim@php.net> wrote:
> On Tue, Aug 23, 2016 at 9:03 AM, Ferenc Kovacs <tyra3l@gmail.com> wrote: >> On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker <cmbecker69@gmx.de> >> wrote: >> >>> On 22.08.2016 at 18:00, Julien Pauli wrote: >>> >>> > I agree this is a BC break and should not stay as-is in source code. >>> >>> I wonder why we have more than 100 lines of "Backward incompatible >>> changes" in the PHP 7.1.0beta3 changelog[1], if BC breaks shouldn't be >>> introduced in a minor release. >>> >> >> That's a bit loaded question, and leads to a broken windows situation, but >> from my understanding some people read >> https://wiki.php.net/rfc/releaseprocess differently: consider some BC >> breaks simply bug fixes, or think that we shouldn't stick to absolutes but >> consider BC breaks on a case-by-case basis. >> personally I think tha >> >> >>> >>> > It makes some testsuites fail, that did not fail before ; thus it breaks >>> things. >>> >>> An estimated 10% (at least) of my *bugfixes* in GD broke at least one >>> PHPT, because the test was broken in the first place. >>> >> >> test failures can be false positive or depending explicitly undefined >> behaviors, but they can be a good indicator when looking for BC breaks. >> as we can see from the previous mails in this thread there are behavior >> changes where the previous behavior was different from what was documented >> so a bit of a grey area. >> >> personally I think that we are in general too lenient with allowing BC >> breaks in 7.1 (even though that I somehow expected this and was arguing for >> a longer release cycle for 7.0 or at least having a clear roadmap for the >> next major version) and we should be more strict about it otherwise we will >> lose the trust we gained from the userland in the last couple of years with >> our release process and versioning. >> >> >> >> -- >> Ferenc Kovács >> @Tyr43l - http://tyrael.hu > > I agree. However in this particular instance emitting an E_DEPRECATED > like I proposed should be helpful. These people who are passing > strings are not getting any behavior out of it - its' completely > ignored. Additionally since its an E_DEPRECATED it's highly unlikely > to actually break something. What do you think?
I'm OK to generate a E_DEPRECATED and continue ignoring the first parameter. And then talk about ways to call parents in child context as ideas, debates, RFCs and patches. I myself can work on patches, but as usual, not on ideas as I can't get the point for such use cases. Julien