A colleague at Digg pointed me to this blog entry:
http://atomized.org/2008/11/more-tough-love-from-php/
To me it looks like a large inconsistency in the way we invoke overloaded handlers for
members vs. methods. I am not sure how it came to be this way, but it's probably something
we should fix. We can either remove the call to __get() on private member access, or add
call to __call() on private method invocation. The former approach presents more of a BC
problem IMHO, so I am advocating the latter. I've attached a simple patch for consideration.
-Andrei
To me it looks like a large inconsistency in the way we invoke overloaded
handlers for
members vs. methods. I am not sure how it came to be this way, but it's
probably something
we should fix. We can either remove the call to __get() on private member
access, or add
call to __call() on private method invocation. The former approach presents
more of a BC
problem IMHO, so I am advocating the latter. I've attached a simple patch
for consideration.
I'd say go ahead, this sounds like common sense to be consistent in
both methods and members.
--
Slan,
David
2009/1/6 David Coallier davidc@php.net:
call to __call() on private method invocation. The former approach presents
more of a BC
problem IMHO, so I am advocating the latter. I've attached a simple patch
for consideration.I'd say go ahead, this sounds like common sense to be consistent in
both methods and members.
Cool, looks like this has gone in already. A couple of comments:
- I think the change has not been made for callbacks (so callbacks
now behave differently from normal method invocations). - Perhaps on 5_3 and HEAD, the equivalent change should be made for
__callStatic()?
Addressing those two points would make things even more consistent.
I'm adding some testcases to illustrate.
--TEST--
Trigger __call() in lieu of non visible methods when called via a callback.
--FILE--
<?php
class C {
protected function prot() { }
private function priv() { }
public function __call($name, $args) {
echo "In __call() for method $name()\n";
}
}
$c = new C;
call_user_func(array($c, 'none'));
call_user_func(array($c, 'prot'));
call_user_func(array($c, 'priv'));
?>
--EXPECTF--
In __call() for method none()
In __call() for method prot()
In __call() for method priv()
--TEST--
Trigger __callStatic() in lieu of non visible methods when called
directly or via a callback.
--FILE--
<?php
class C {
protected static function prot() { }
private static function priv() { }
public static function __callStatic($name, $args) {
echo "In __callStatic() for method $name()\n";
}
}
echo "Using direct method invocation:\n";
C::none();
C::prot();
C::priv();
echo "\nUsing callbacks:\n";
call_user_func(array('C', 'none'));
call_user_func(array('C', 'prot'));
call_user_func(array('C', 'priv'));
?>
--EXPECTF--
Using direct method invocation:
In __callStatic() for method none()
In __callStatic() for method prot()
In __callStatic() for method priv()
Using callbacks:
In __callStatic() for method none()
In __callStatic() for method prot()
In __callStatic() for method priv()
Robin Fernandes wrote:
Cool, looks like this has gone in already. A couple of comments:
- I think the change has not been made for callbacks (so callbacks
now behave differently from normal method invocations).- Perhaps on 5_3 and HEAD, the equivalent change should be made for
__callStatic()?Addressing those two points would make things even more consistent.
I'm adding some testcases to illustrate.
I fixed the callback case in 5_2. Need to talk to Dmitry about 5_3 and HEAD because
zend_is_callable_check_func() is damn complex now.
-Andrei
Addressing those two points would make things even more consistent.
I'm adding some testcases to illustrate.I fixed the callback case in 5_2. Need to talk to Dmitry about 5_3 and HEAD
because zend_is_callable_check_func() is damn complex now.-Andrei
Thanks!
I noticed another inconsistency with this new behaviour: since bug
42937 was fixed, calling a non-existent method with the scope
resolution operator (::) results in __call() being invoked if the
current object scope is compatible with the target scope. I think this
should now be adapted to apply when calling a non-visible method with
::.
Patch against 5_2 with test case: http://pastebin.com/f7ee7a9fc
Regards,
Robin