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 написал:
It is still ignored if it is a valid object, so the current beta's
behaviour doesn't actually make a lot of sense.
Well, see https://3v4l.org/n8cad. Only as of 7.1.0beta3 ::invoke()
matches the behavior of ::invokeArgs().OK, so that's a further inconsistency. The current version still doesn't
actually make sense, though. If you want to validate that the correct
argument is being passed, then the only value allowed should be null.If so desired, I can revert that commit, but I wouldn't be happy with
sticking with a completely unused parameter, which obviously has been
and still is misunderstood.The parameter is still unused, and can be any object. The only sane
validation would be to check explicitly for null, which is documented as
the correct argument for both invoke() and invokeArgs().Regards,
Rowan Collins
[IMSoP]
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:
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 написал:
It is still ignored if it is a valid object, so the current beta's
behaviour doesn't actually make a lot of sense.
Well, see https://3v4l.org/n8cad. Only as of 7.1.0beta3 ::invoke()
matches the behavior of ::invokeArgs().OK, so that's a further inconsistency. The current version still doesn't
actually make sense, though. If you want to validate that the correct
argument is being passed, then the only value allowed should be null.If so desired, I can revert that commit, but I wouldn't be happy with
sticking with a completely unused parameter, which obviously has been
and still is misunderstood.The parameter is still unused, and can be any object. The only sane
validation would be to check explicitly for null, which is documented as
the correct argument for both invoke() and invokeArgs().Regards,
Rowan Collins
[IMSoP]
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
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
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]
On Mon, Aug 22, 2016 at 10:30 AM, Christoph M. Becker cmbecker69@gmx.de
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.
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]
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#L2163Before 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]--
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.
Anyone oppose to emitting
E_DEPRECATEDfor 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 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#L2163Before 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_DEPRECATEDfor 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
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_DEPRECATEDfor 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ä
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#L2163Before 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_DEPRECATEDfor 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 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#L2163Before 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_DEPRECATEDfor 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/8735d6b72de0edc3b370245737c0f4f204fcbea0But 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
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#L2163Before 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_DEPRECATEDfor 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/8735d6b72de0edc3b370245737c0f4f204fcbea0But 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
NULLas 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
Am 23.08.2016 um 14:51 schrieb Julien Pauli:
I suggest to deprecate all other types than
NULLas 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.
I don't think it's good to introduce this shortcut, which is of limited
value anyway. It might be better to have something like
::invokeStatic() which wouldn't require passing a NULL as first parameter.
--
Christoph M. Becker
2016-08-23 14:51 GMT+03:00 Christoph M. Becker cmbecker69@gmx.de:
I suggest to deprecate all other types than
NULLas 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?
On Tue, Aug 23, 2016 at 2:56 PM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:
2016-08-23 14:51 GMT+03:00 Christoph M. Becker cmbecker69@gmx.de:
I suggest to deprecate all other types than
NULLas 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?
It makes no sense to change the scope of the method.
A method is not a closure.
A method is bound to a class , a closure is by default unbound. A
method cannot be unbound. A method cannot change from one class to
another.
This :
class A { public static function foo() { } }
class B extends A { public static function foo() { } }
$a = new reflectionMethod('A', 'foo');
$a->invoke('B');
would lead to the exact equivalent as this with my patch :
$b = new reflectionMethod('B', 'foo');
$b->invoke();
So I don't see why we should change and patch the code to support such
a strange use case ...
If you want to call B's foo , create a ReflectionMethod from B, not from A.
Julien
2016-08-23 16:01 GMT+03:00 Julien Pauli jpauli@php.net:
This :
class A { public static function foo() { } }
class B extends A { public static function foo() { } }$a = new reflectionMethod('A', 'foo');
$a->invoke('B');
It's perfect way for me.
Just to be sure: in this case we will invoke A::foo() method, but the scope
(static::class) will be 'B', right? If yes, then everything is ok and it
will be compatible with all existing code but will have an additional
meaning for calling methods with children's scope.
On Tue, Aug 23, 2016 at 3:07 PM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:
2016-08-23 16:01 GMT+03:00 Julien Pauli jpauli@php.net:
This :
class A { public static function foo() { } }
class B extends A { public static function foo() { } }$a = new reflectionMethod('A', 'foo');
$a->invoke('B');It's perfect way for me.
Just to be sure: in this case we will invoke A::foo() method, but the scope
(static::class) will be 'B', right? If yes, then everything is ok and it
No, How can this be ? B extends A , not A extends B ...
In this case, B'sfoo() will be called , which is the equivalent to
having a reflectionMethod from B , so useless to me.
Julien
2016-08-23 16:13 GMT+03:00 Julien Pauli jpauli@php.net:
No, How can this be ? B extends A , not A extends B ...
In this case, B'sfoo() will be called , which is the equivalent to
having a reflectionMethod from B , so useless to me.
Ok, how can I invoke Parent::whoami() with reflection and get exactly
"Child" as an output?
class Parent {
public static function whoami() {
echo static::class;
}
}
class Child extends Parent {
public static function whoami() {
echo "Don't call me now!';
parent::whoami();
}
}
2016-08-23 16:13 GMT+03:00 Julien Pauli <jpauli@php.net
mailto:jpauli@php.net>:No, How can this be ? B extends A , not A extends B ... In this case, B'sfoo() will be called , which is the equivalent to having a reflectionMethod from B , so useless to me.Ok, how can I invoke Parent::whoami() with reflection and get exactly
"Child" as an output?
I've no idea why you'd want to, but note that using get_class($this),
you can actually get that for a non-static method: https://3v4l.org/Rc7Dk
class MyParent {
public function whoami() {
echo get_class($this);
}
}
class MyChild extends MyParent {
public function whoami() {
echo "Don't call me now!";
parent::whoami();
}
}
$m = new reflectionMethod('MyParent', 'whoami');
$m->invoke(new MyChild);
So there is a gap in functionality between static and non-static methods
here. I'm not sure reinterpreting a parameter which has always been
ignored is the best way to solve it though.
Regards,
Rowan Collins
[IMSoP]
On Tue, Aug 23, 2016 at 3:20 PM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:
2016-08-23 16:13 GMT+03:00 Julien Pauli jpauli@php.net:
No, How can this be ? B extends A , not A extends B ...
In this case, B'sfoo() will be called , which is the equivalent to
having a reflectionMethod from B , so useless to me.Ok, how can I invoke Parent::whoami() with reflection and get exactly
"Child" as an output?class Parent {
public static function whoami() {
echo static::class;
}
}class Child extends Parent {
public static function whoami() {
echo "Don't call me now!';
parent::whoami();
}
}
My patch allows that, but I can't find a use case to it.
Try it.
Julien
2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
My patch allows that, but I can't find a use case to it.
My use case for that was decorating of static methods with additional
behaviour, so it's possible to cache the result of static methods, etc.
Unfortunately, reflection API doesn't provide me such an ability, so I
decided to switch to the combination of special closure with
forward_static_call_array() and binding it to the desired scope.
2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
Try it.
Yes, sure, I will check. However as I say earlier, now I use closure
extraction and scope binding as more natural way to invoke such methods. So
missing scope argument for static methods and ReflectionMethod->invoke() is
a nice thing to have, but not a critical one.
On Tue, Aug 23, 2016 at 7:56 AM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:
2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
My patch allows that, but I can't find a use case to it.
My use case for that was decorating of static methods with additional
behaviour, so it's possible to cache the result of static methods, etc.
Unfortunately, reflection API doesn't provide me such an ability, so I
decided to switch to the combination of special closure with
forward_static_call_array()and binding it to the desired scope.2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
Try it.
Yes, sure, I will check. However as I say earlier, now I use closure
extraction and scope binding as more natural way to invoke such methods. So
missing scope argument for static methods and ReflectionMethod->invoke() is
a nice thing to have, but not a critical one.
Seems to me you should be calling getClosure() and using
Closure::bindTo before invoking it. ReflectionMethod::invoke is really
just a short-cut for a common case; I don't think it's reasonable to
have it mirror the Closure API just to save one method call to get a
Closure.
On Tue, Aug 23, 2016 at 7:56 AM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
My patch allows that, but I can't find a use case to it.
My use case for that was decorating of static methods with additional
behaviour, so it's possible to cache the result of static methods, etc.
Unfortunately, reflection API doesn't provide me such an ability, so I
decided to switch to the combination of special closure with
forward_static_call_array()and binding it to the desired scope.2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
Try it.
Yes, sure, I will check. However as I say earlier, now I use closure
extraction and scope binding as more natural way to invoke such methods. So
missing scope argument for static methods and ReflectionMethod->invoke() is
a nice thing to have, but not a critical one.Seems to me you should be calling getClosure() and using
Closure::bindTo before invoking it. ReflectionMethod::invoke is really
just a short-cut for a common case; I don't think it's reasonable to
have it mirror the Closure API just to save one method call to get a
Closure.
This cannot work.
There is a special use case forbidding that.
"Cannot rebind scope of closure created by
ReflectionFunctionAbstract::getClosure()"
Julien
On Tue, Aug 23, 2016 at 7:56 AM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
My patch allows that, but I can't find a use case to it.
My use case for that was decorating of static methods with additional
behaviour, so it's possible to cache the result of static methods, etc.
Unfortunately, reflection API doesn't provide me such an ability, so I
decided to switch to the combination of special closure with
forward_static_call_array()and binding it to the desired scope.2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
Try it.
Yes, sure, I will check. However as I say earlier, now I use closure
extraction and scope binding as more natural way to invoke such methods. So
missing scope argument for static methods and ReflectionMethod->invoke() is
a nice thing to have, but not a critical one.Seems to me you should be calling getClosure() and using
Closure::bindTo before invoking it. ReflectionMethod::invoke is really
just a short-cut for a common case; I don't think it's reasonable to
have it mirror the Closure API just to save one method call to get a
Closure.This cannot work.
There is a special use case forbidding that.
"Cannot rebind scope of closure created by
ReflectionFunctionAbstract::getClosure()"Julien
I know this is a stability concern for internal classes but is there
anything really preventing this from user-land classes?
On Tue, Aug 23, 2016 at 7:56 AM, Alexander Lisachenko
lisachenko.it@gmail.com wrote:2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
My patch allows that, but I can't find a use case to it.
My use case for that was decorating of static methods with additional
behaviour, so it's possible to cache the result of static methods, etc.
Unfortunately, reflection API doesn't provide me such an ability, so I
decided to switch to the combination of special closure with
forward_static_call_array()and binding it to the desired scope.2016-08-23 16:40 GMT+03:00 Julien Pauli jpauli@php.net:
Try it.
Yes, sure, I will check. However as I say earlier, now I use closure
extraction and scope binding as more natural way to invoke such methods. So
missing scope argument for static methods and ReflectionMethod->invoke() is
a nice thing to have, but not a critical one.Seems to me you should be calling getClosure() and using
Closure::bindTo before invoking it. ReflectionMethod::invoke is really
just a short-cut for a common case; I don't think it's reasonable to
have it mirror the Closure API just to save one method call to get a
Closure.This cannot work.
There is a special use case forbidding that.
"Cannot rebind scope of closure created by
ReflectionFunctionAbstract::getClosure()"Julien
I know this is a stability concern for internal classes but is there
anything really preventing this from user-land classes?
I remember problems and bugs around this , but I don't remember the
exact use-case why we forbid that.
I guess Dmitry, Xinchen or Nikic could remember.
Julien
Ok, how can I invoke Parent::whoami() with reflection and get exactly
"Child" as an output?class Parent {
public static function whoami() {
echo static::class;
}
}class Child extends Parent {
public static function whoami() {
echo "Don't call me now!';
parent::whoami();
}
}
Could you do it without reflection, then? I don't think so.
Thus, I think your "use case" is broken by design.
If you need to call Parent::whoami with the Child scope,
maybe you shouldn't override the method in Child class at all.
If this kind of functionality is generally needed,
maybe it could be borrowed from C++:
obj_b->A::whoami();
Translated to PHP/Reflection:
new ReflectionMethod("B", "A::whoami")->invoke();
--
Lauri Kenttä
2016-08-23 18:05 GMT+03:00 Lauri Kenttä lauri.kentta@gmail.com:
Could you do it without reflection, then? I don't think so.
Thus, I think your "use case" is broken by design.
FYI: This use case is used by Go! AOP Framework and AspectMock libraries
that can mock even static methods in the classes, allowing testing legacy
code and singletons. If you are interested in how it works, then you could
look at this example: https://3v4l.org/TbS5V
2016-08-23 18:05 GMT+03:00 Lauri Kenttä lauri.kentta@gmail.com:
Could you do it without reflection, then? I don't think so.
Thus, I think your "use case" is broken by design.FYI: This use case is used by Go! AOP Framework and AspectMock
libraries that can mock even static methods in the classes, allowing
testing legacy code and singletons. If you are interested in how it
works, then you could look at this example: https://3v4l.org/TbS5V
Ok, so I was wrong about it being impossible. Sorry about that.
However, if you look at that example, does it look good and solid?
It looks more like a weird hack to me.
So I stand by my words: if there's a legitimate use for calling
parent static methods with child context, there should also be
simpler ways than reflection or strange closure hacks for doing it.
There should be a syntax like ChildClass::ParentClass::method(),
or at least call_user_func(["ChildClass", "ParentClass", "method"]),
or call_user_func(["ChildClass", "ParentClass::method"]), since the
ParentClass can be considered a qualifier for the method name.
--
Lauri Kenttä
For static methods this could be object (why not - it will be ignored)
Allowing a parameter and then completely ignoring it is what got us into
this situation in the first place.
Why does this...
$a = new reflectionMethod('A', 'foo');
$a->invoke(new DateTime);
...make any more sense than this?
$a = new reflectionMethod('A', 'foo');
$a->invoke(42);
If you want to allow an object to be passed in, it should act the same
way as a string - specify the binding context:
$a = new reflectionMethod('A', 'foo');
$a->invoke(new B); // equivalent to $a->invoke('B');
It should also go through the same check as it would for a non-static
method: must be an instance of the class being reflected.
Regards,
Rowan Collins
[IMSoP]
On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker cmbecker69@gmx.de
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
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
On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker cmbecker69@gmx.de
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 thaIt 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?
On Mon, Aug 22, 2016 at 6:30 PM, Christoph M. Becker cmbecker69@gmx.de
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 thaIt 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.huI 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 anE_DEPRECATEDit'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