Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39631
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: <48970FBC.4060401@zend.com>
Date: Mon, 04 Aug 2008 18:18:36 +0400
User-Agent: Thunderbird 2.0.0.16 (Windows/20080708)
MIME-Version: 1.0
To: Christian Seiler <chris_se@gmx.net>
CC: Etienne Kneuss <colder@php.net>, 
 PHP Internals List <internals@lists.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 Christan,

Could you please look into this patch.
I'm not sure if explicit declaration of Closure::__invoke() is good 
idea. As it cannot provide proper argument information.

May be the patch which you propose already solves this problem for 
reflection. (I didn't have time to look into it).

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? )