# [PATCH] Closures and reflection improvements

- Thread: https://externals.io/message/39265
- Original archive: https://news-web.php.net/php.internals/39265
- Started by Christian Seiler on 2008-07-24T11:24:01+00:00
- Messages: 20
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #39265 — Christian Seiler — 2008-07-24T11:24:01+00:00

URL: https://externals.io/message/39265#39265

Hi,

I'd like to add the following patch to reflection before the feature
freeze, because it does not change any current reflection behaviour but
adds quite a bit of good stuff for closures.

With the current closure implementation, __invoke always has the same
signature as the original lambda function, this makes passing parameters
per reference possible. But this does not show in current reflection:

$o = new ReflectionObject (function ($a) {});
$m = $o->getMethod ('__invoke');
var_dump ($m->getNumberOfParameters());

Currently, this does a lookup in the class function table. But the
signature of the method found in the function table has no parameters.
For the normal engine to have a different signature for a specific
closure, the get_method handler is used.

My patch does the following:

  * If a reflection method named '__invoke' is requested for an object
    of the 'Closure' class (in any way possible: via constructor,
    via ReflectionObject->getMethod, via ReflectionParameter
    constructor, ...), the correct invoke signature is used by
    reflection.

  * If the same method for the class 'Closure' (without an object)
    is requested, the standard signautre (no parameters) will be used.

  * If you pass a callable object (closure or otherwise) as the only
    parameter to the ReflectionMethod constructor, __invoke will be
    used as default method. Example:
     $m = new ReflectionMethod ($closure);
    is identical to
     $m = new ReflectionMethod ($closure, '__invoke');

For Post-5.3 we could even think of adding a new object handler for
retrieving methods for reflection generally (get_method will not work
correctly since the std_get_method handler does scope checking and
prints error messages) and thus allowing e.g. COM, Java or SOAP objects
to provide useful dynamic information for reflection. But to keep the
changes in 5.3 as non-invasive as possible, it is probably best to have
this single exception for closures.

You can find the patch here:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch
http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

The patch is fairly straight-forward and should be easily reviewed.

Here are some tests for this patch:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-tests-5.3.tar.gz
http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-tests-6.tar.gz

I also found a segfault in *all* PHP versions in the ReflectionParameter
constructor. The above patches already fix that, but here's a patch that
fixes this for PHP 5.2:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

For the segfault I also created following test case (works with 5.2, 5.3
and 6.0), NOT included in the above .tar.gzs.

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

Regards,
Christian

---

## #39324 — Christian Seiler — 2008-07-25T15:41:49+00:00

URL: https://externals.io/message/39265#39324 · In reply to: #39265

Hi,

> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch 
> 
> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch 

The last CVS commit for apply_func_t and TSRMLS_CC conflicted with the
patches, I merged the conflicting line manually and reuploaded the
patches to the same location.

Regards,
Christian

---

## #39333 — Marcus Boerger — 2008-07-25T18:56:42+00:00

URL: https://externals.io/message/39265#39333 · In reply to: #39324

Hello Christian,

  patch looks fine and should go in.

  do you think ReflectionMethod::__construct implementation could be done
using parameterparsing 'f' rather than the spcial case 'o'? Unfortunately
we would still need the zs version unless we add another 'F' parameter
parsing that allows this two parameter case as well.

marcus

Friday, July 25, 2008, 5:41:49 PM, you wrote:

> Hi,

> > […]

> The last CVS commit for apply_func_t and TSRMLS_CC conflicted with the
> patches, I merged the conflicting line manually and reuploaded the
> patches to the same location.

> Regards,
> Christian




Best regards,
 Marcus

---

## #39334 — Christian Seiler — 2008-07-25T19:01:13+00:00

URL: https://externals.io/message/39265#39334 · In reply to: #39333

Hi Marcus,

>   patch looks fine and should go in.

Thanks.

> 
>   do you think ReflectionMethod::__construct implementation could be done
> using parameterparsing 'f' rather than the spcial case 'o'?

The Problem with 'f' is that it will accept every callback, even normal
functions, so that would kind of break the idea of having
ReflectionMethod different than ReflectionFunction.

So in my eyes, using 'o' for now is the sanest approach.

Regards,
Christian

---

## #39336 — Marcus Boerger — 2008-07-25T19:12:36+00:00

URL: https://externals.io/message/39265#39336 · In reply to: #39334

Hello Christian,

Friday, July 25, 2008, 9:01:13 PM, you wrote:

> Hi Marcus,

> > […]

> Thanks.

> > […]

> The Problem with 'f' is that it will accept every callback, even normal
> functions, so that would kind of break the idea of having
> ReflectionMethod different than ReflectionFunction.

> So in my eyes, using 'o' for now is the sanest approach.

I see. So if we ever want to fix it we need 'm' for method then :-)

Best regards,
 Marcus

---

## #39339 — Lukas Kahwe Smith — 2008-07-25T21:00:18+00:00

URL: https://externals.io/message/39265#39339 · In reply to: #39333

> Hello Christian,
>
>  patch looks fine and should go in.
>
>  do you think ReflectionMethod::__construct implementation could be  
> done
> using parameterparsing 'f' rather than the spcial case 'o'?  
> Unfortunately
> we would still need the zs version unless we add another 'F' parameter
> parsing that allows this two parameter case as well.
>
> marcus
>
> Friday, July 25, 2008, 5:41:49 PM, you wrote:
>
> > […]
>
> > […]


if this isnt commited yet .. could someone handle this ASAP?

Lukas Kahwe Smith
mls@pooteeweet.org

---

## #39598 — Dmitry Stogov — 2008-08-04T07:33:08+00:00

URL: https://externals.io/message/39265#39598 · In reply to: #39324

Hi Marcus,

I have no objections against ZE part of the patch.
If you like ext/reflection part please commit the whole patch.

Thanks. Dmitry.

Christian Seiler wrote:
> Hi,
> 
> > […]
> 
> 
> The last CVS commit for apply_func_t and TSRMLS_CC conflicted with the
> patches, I merged the conflicting line manually and reuploaded the
> patches to the same location.
> 
> Regards,
> Christian

---

## #39623 — johannes@php.net — 2008-08-04T10:59:13+00:00

URL: https://externals.io/message/39265#39623 · In reply to: #39598

Christian,


> > […]

without applying and testing the patch:

-	zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, (apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
+	zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, (apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);

that looks a bit strange, probably you want 4 instead of 3 now?

johannes

---

## #39634 — Christian Seiler — 2008-08-04T16:40:32+00:00

URL: https://externals.io/message/39265#39634 · In reply to: #39598

Hi Dmitry, Hi Marcus,

> I have no objections against ZE part of the patch.
> If you like ext/reflection part please commit the whole patch.

Due to your cleanup wrt. handlers the reflection part will currently
(probably) segfault. I'll post an updated patch for this tomorrow.

Regards,
Christian

---

## #39777 — Marcus Boerger — 2008-08-08T21:48:37+00:00

URL: https://externals.io/message/39265#39777 · In reply to: #39623

Hello Christian,

  I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.


Hello Johannes,

  I addressed the thing you spotted.


best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

> Christian,

> 
> > […]

> without applying and testing the patch:

> -       zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
> (apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
> +       zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
> (apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);

> that looks a bit strange, probably you want 4 instead of 3 now?

> johannes





Best regards,
 Marcus

---

## #39782 — Marcus Boerger — 2008-08-08T22:34:59+00:00

URL: https://externals.io/message/39265#39782 · In reply to: #39777

Hello Christian, Johannes,

Friday, August 8, 2008, 11:48:37 PM, you wrote:

> Hello Christian,

>   I updated your patch for 5.3 due to recent changes. It works pretty fine
> for me. Care to test again, especially with your new tests? Note that test
> ext/reflection/tests/closures_001.phpt does not work for me and I have no
> clue why. Well, besides that obviously the closure class no longer has an
> entry for __closure, so it is more a question of how to know when to add
> this. Also, it appears you do not have a cvs account, or did I overlook
> something?

> Modified patch attached, including the updated reflection tests.

Ok, it is actually easy to fix, we just check whether the thing is a
closure and add the method manually, done. New patch attached.

Johannes, this also adresses the switch from `E_ERROR` to `E_RECOVERABLE_ERROR` 
for the closure handlers.

best regards
marcus

> Monday, August 4, 2008, 12:59:13 PM, you wrote:

> > […]

> > […]

> > […]

> > […]

> > […]

> > […]





> Best regards,
>  Marcus



Best regards,
 Marcus

---

## #39813 — Marcus Boerger — 2008-08-10T22:51:38+00:00

URL: https://externals.io/message/39265#39813 · In reply to: #39782

Hello Johannes, Christian, Felipe,

  and here 5.3 and 6.0 versions that also have most memleaks fixed.

marcus

Saturday, August 9, 2008, 12:34:59 AM, you wrote:

> Hello Christian, Johannes,

> Friday, August 8, 2008, 11:48:37 PM, you wrote:

> > […]

> > […]

> > […]

> Ok, it is actually easy to fix, we just check whether the thing is a
> closure and add the method manually, done. New patch attached.

> Johannes, this also adresses the switch from `E_ERROR` to `E_RECOVERABLE_ERROR` 
> for the closure handlers.

> best regards
> marcus

> > […]

> > […]

> > […]

> > […]

> > […]

> > […]

> > […]





> > […]



> Best regards,
>  Marcus



Best regards,
 Marcus

---

## #39820 — Dmitry Stogov — 2008-08-11T08:51:44+00:00

URL: https://externals.io/message/39265#39820 · In reply to: #39813

Hi Marcus,

I committed the ZE part of your patch with exception for

zend_error(E_ERROR, "Cannot destroy active lambda function");

Catching this error may cause memory corruption and crash.

Thanks. Dmitry.

Marcus Boerger wrote:
> Hello Johannes, Christian, Felipe,
> 
>   and here 5.3 and 6.0 versions that also have most memleaks fixed.
> 
> marcus
> 
> Saturday, August 9, 2008, 12:34:59 AM, you wrote:
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> > […]
> 
> 
> 
> 
> 
> > […]
> 
> 
> 
> > […]
> 
> 
> 
> Best regards,
>  Marcus

---

## #39828 — Marcus Boerger — 2008-08-11T10:23:57+00:00

URL: https://externals.io/message/39265#39828 · In reply to: #39820

Hello Dmitry,

Monday, August 11, 2008, 10:51:44 AM, you wrote:

> Hi Marcus,

> I committed the ZE part of your patch with exception for

> zend_error(E_ERROR, "Cannot destroy active lambda function");

> Catching this error may cause memory corruption and crash.

Alright, didn't see that immediately.

> Thanks. Dmitry.
Thanks marcus

> Marcus Boerger wrote:
> > […]




Best regards,
 Marcus

---

## #39830 — Marcus Boerger — 2008-08-11T12:45:02+00:00

URL: https://externals.io/message/39265#39830 · In reply to: #39828

Hello guys,

  all submitted now. Thanks everyone for their help and especially
  Christian for getting this started.

marcus

Monday, August 11, 2008, 12:23:57 PM, you wrote:

> Hello Dmitry,

> Monday, August 11, 2008, 10:51:44 AM, you wrote:

> > […]

> > […]

> > […]

> > […]

> Alright, didn't see that immediately.

> > […]
> Thanks marcus

> > […]




> Best regards,
>  Marcus





Best regards,
 Marcus

---

## #39834 — Christian Seiler — 2008-08-11T18:43:31+00:00

URL: https://externals.io/message/39265#39834 · In reply to: #39830

Hi,

>   all submitted now. Thanks everyone for their help and especially
>   Christian for getting this started.

Thanks for making my patch work after the modifications in
zend_closures.c. I'm sorry I couldn't do it myself but I was kind of
busy the last few days and the "simple fix" created memory leaks and I
didn't want to post a knowingly broken patch.

Anyway, there are still segfaults in the current version:

$foo = function ($a) {};
$o = new ReflectionMethod ($foo);
$p = $o->getParameters ();
unset ($o);
$m = $p[0]->getDeclaringFunction ();
Reflection::export ($m);

$foo = function ($a) {};
$p = new ReflectionParameter ($foo, 'a');
$m = $p->getDeclaringFunction ();
unset ($p);
Reflection::export ($m);

This is due to the fact that getParameters() / getDeclaringFunction()
don't copy the fptr and fptr->common.function_name pointers if it's a
closure. Here are patches for PHP 5.3 and HEAD that fix the problem:

http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-5.3.patch
http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-6.patch

Also, the following test that I already posted should be added to
PHP_5_2, PHP_5_3 and HEAD:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

The test already works for PHP_5_3 (since the fix for that was included
in my original patch) but a segfault still occurs in PHP_5_2 and HEAD,
my patch that I already posted fixes that problem for 5.2:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

My original patch also fixed the problem for HEAD, I readded that part
to the above segfault patch for HEAD.

> Also, it appears you do not have a cvs account, or did I overlook
> something?

No, I don't. Dmitry also mentioned that - should I request one?

Regards,
Christian

---

## #39835 — Marcus Boerger — 2008-08-11T19:01:39+00:00

URL: https://externals.io/message/39265#39835 · In reply to: #39834

Hello Christian,

Monday, August 11, 2008, 8:43:31 PM, you wrote:

> Hi,

> > […]

> Thanks for making my patch work after the modifications in
> zend_closures.c. I'm sorry I couldn't do it myself but I was kind of
> busy the last few days and the "simple fix" created memory leaks and I
> didn't want to post a knowingly broken patch.

> Anyway, there are still segfaults in the current version:

> $foo = function ($a) {};
> $o = new ReflectionMethod ($foo);
> $p = $o->getParameters ();
> unset ($o);
> $m = $p[0]->getDeclaringFunction ();
> Reflection::export ($m);

> $foo = function ($a) {};
> $p = new ReflectionParameter ($foo, 'a');
> $m = $p->getDeclaringFunction ();
> unset ($p);
> Reflection::export ($m);

> This is due to the fact that getParameters() / getDeclaringFunction()
> don't copy the fptr and fptr->common.function_name pointers if it's a
> closure. Here are patches for PHP 5.3 and HEAD that fix the problem:

> http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-5.3.patch
> http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-6.patch

> Also, the following test that I already posted should be added to
> PHP_5_2, PHP_5_3 and HEAD:

> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

> The test already works for PHP_5_3 (since the fix for that was included
> in my original patch) but a segfault still occurs in PHP_5_2 and HEAD,
> my patch that I already posted fixes that problem for 5.2:

> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

> My original patch also fixed the problem for HEAD, I readded that part
> to the above segfault patch for HEAD.

> > […]

> No, I don't. Dmitry also mentioned that - should I request one?

Yes, please do and the seg fix patch should have a function to do the
copying as it is not complete, so we can add to it as necessary.

Best regards,
 Marcus

---

## #39837 — Marcus Boerger — 2008-08-11T19:40:21+00:00

URL: https://externals.io/message/39265#39837 · In reply to: #39834

Hello Christian,

Monday, August 11, 2008, 8:43:31 PM, you wrote:

> Hi,

> > […]

> Thanks for making my patch work after the modifications in
> zend_closures.c. I'm sorry I couldn't do it myself but I was kind of
> busy the last few days and the "simple fix" created memory leaks and I
> didn't want to post a knowingly broken patch.

> Anyway, there are still segfaults in the current version:

> $foo = function ($a) {};
> $o = new ReflectionMethod ($foo);
> $p = $o->getParameters ();
> unset ($o);
> $m = $p[0]->getDeclaringFunction ();
> Reflection::export ($m);

> $foo = function ($a) {};
> $p = new ReflectionParameter ($foo, 'a');
> $m = $p->getDeclaringFunction ();
> unset ($p);
> Reflection::export ($m);

> This is due to the fact that getParameters() / getDeclaringFunction()
> don't copy the fptr and fptr->common.function_name pointers if it's a
> closure. Here are patches for PHP 5.3 and HEAD that fix the problem:

> http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-5.3.patch
> http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-6.patch

> Also, the following test that I already posted should be added to
> PHP_5_2, PHP_5_3 and HEAD:

> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

> The test already works for PHP_5_3 (since the fix for that was included
> in my original patch) but a segfault still occurs in PHP_5_2 and HEAD,
> my patch that I already posted fixes that problem for 5.2:

> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

> My original patch also fixed the problem for HEAD, I readded that part
> to the above segfault patch for HEAD.

> > […]

> No, I don't. Dmitry also mentioned that - should I request one?

Account approved and access granted. Please provide the function copying in
a function next to _free_function() and care for parameter info as well
(yeah sorry for the additional work). And then, happy submitting.

Also for your next patches (cvs di -N) allows to ship the new files in the
patch and (cvs di -Np) shows the context, which makes reading the patches
easier.

Best regards,
 Marcus

---

## #39844 — Christian Seiler — 2008-08-11T22:34:41+00:00

URL: https://externals.io/message/39265#39844 · In reply to: #39837

Hi Marcus,

> Account approved and access granted.

Thanks!

> Please provide the function copying in a function next to _free_function()

I just committed that (and included some tests). I hope I didn't do
anything wrong in the process, if so, feel free to do so.

> and care for parameter info as well

I assume with parameter info you mean arg_info? That isn't necessary and
actually harmful since the arg_info is not copied but simply referenced
via a C pointer in closures (zend_get_closure_invoke_method copies the
entire function structure, including the pointer to arg_info and only
changes some flags the function name). There is no need to copy or
destroy arg_info in reflection.

> Also for your next patches (cvs di -N) allows to ship the new files in the
> patch and (cvs di -Np) shows the context, which makes reading the patches
> easier.

Ah, thanks, I added that to my .cvsrc.

Regards,
Christan

---

## #39846 — Marcus Boerger — 2008-08-11T23:13:11+00:00

URL: https://externals.io/message/39265#39846 · In reply to: #39844

Hello Christian,

Tuesday, August 12, 2008, 12:34:41 AM, you wrote:

> Hi Marcus,

> > […]

> Thanks!

> > […]

> I just committed that (and included some tests). I hope I didn't do
> anything wrong in the process, if so, feel free to do so.

> > […]

> I assume with parameter info you mean arg_info? That isn't necessary and
> actually harmful since the arg_info is not copied but simply referenced
> via a C pointer in closures (zend_get_closure_invoke_method copies the
> entire function structure, including the pointer to arg_info and only
> changes some flags the function name). There is no need to copy or
> destroy arg_info in reflection.

Oh, cool. I thought we'd have to do more stuff.

> > […]

> Ah, thanks, I added that to my .cvsrc.

> Regards,
> Christan




Best regards,
 Marcus