Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39602
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.163 as permitted sender)
Message-ID: <4896B82D.2050500@zend.com>
Date: Mon, 04 Aug 2008 12:05:01 +0400
User-Agent: Thunderbird 2.0.0.16 (Windows/20080708)
MIME-Version: 1.0
To: Etienne Kneuss <colder@php.net>
CC: PHP Internals List <internals@lists.php.net>, 
 Marcus Boerger <helly@php.net>
References: <e666c3890808021036p58de549as422bec8471b7a992@mail.gmail.com> <e666c3890808031503v5a966b75y4d81c750b43b64b7@mail.gmail.com>
In-Reply-To: <e666c3890808031503v5a966b75y4d81c750b43b64b7@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: __invoke concerns
From: dmitry@zend.com (Dmitry Stogov)

Hi Etienne,

In general the patch looks fine.

The only thing I don't like is explicit declaration of Closure::__invode 
method. It won't have proper arg_info.

Thanks. Dmitry.



Etienne Kneuss wrote:
> Hello,
> 
> On Sat, Aug 2, 2008 at 7:36 PM, Etienne Kneuss <colder@php.net> wrote:
>> Hi,
>>
>> this is probably not the best time to raise concerns about __invoke
>> (closures) now that alpha1 is already realeased, but I believe it's
>> worth it.
>>
>> 1) I don't believe that having it thrown as another of those magic
>> method is a good idea. Rather, I'd like to have it represented by an
>> interface: Invokable. That way, type hints/checks can be done in user
>> land in a sane matter:
>>
>> function foo(Invokable $obj) {
>>    $obj();
>> }
>>
>> if ($foo instanceof Invokable) $foo();
>>
>> etc..
>>
>> 2) Do we really want __invoke's argument to be mapped to caller
>> arguments. Providing an array of arguments, ala __call(Static) sounds
>> more consistent.
>> class A { public function __invoke($arg) {var_dump($arg); }} $a = new
>> A; $a(1,2); // int(1), currently. IMHO it should be array(1,2)
>>
>>
>> 3) Do we really want to allow both static and non-static versions of __invoke ?
>> class A { public static function __invoke($args) { .. }} $a = new A;
>> $a(); being a static call to __invoke doesn't make much sense to me.
>>
>>
>> I hope these issues can be discussed and maybe addressed before a
>> final 5.3 release. I'm willing to do patches for all three concerns if
>> I sense a positive feeling towards this.
>>
>> Regards,
>>
>> --
>> Etienne Kneuss
>> http://www.colder.ch
>>
>> Men never do evil so completely and cheerfully as
>> when they do it from a religious conviction.
>> -- Pascal
>>
> 
> After looking deeper into it, I noticed that this interface brings a
> lot of problems:
> 
> 1) With the interface, the prototype is fixed.
> Which means that the following are no longer possible:
> 
> Class A {
> public function &__invoke(&$a, $b, $c) { return $a; }
> }
> $a = new A; $e =& $a($b,$c, $d);
> 
> 2) __invoke($args) seems more consistent, and would give a consistent
> prototype, but
> 2.1) references are no longer possible (1)
> 2.2) __invoke of actual closures/lambdas needs to map parameters for
> $a = function($b,$c){}; to work properly. I don't believe $cl =
> function($args){..} is something we want
> 
> So, with those counter-concerns in mind, this Invokable interface is
> no longer implementable, IMO.
> 
> However, I'd still like to make closures more flexible and
> internals-friendly by implementing zend_get_closure as a handler.
> The only (tiny) issue here is that we export a bit more
> closure-material in the engine, and it's no longer so much
> self-contained in zend_closures.
> 
> Patches:
> http://patches.colder.ch/Zend/zend_get_closure_handler_53.patch?markup
> http://patches.colder.ch/Zend/zend_get_closure_handler_HEAD.patch?markup
> 
> ps: this patches also expose the __invoke method for reflection. (Or
> is there an actual reason behind not exposing it? )