Forbid rebinding scope of closures created by ReflectionFunctionAbstract::getClosure()

Hello, internals!

Just noticed this thread and want to clarify some things with getClosure()
method. If i understood correctly, ReflectionFunctionAbstract::getClosure()
should return non-rebindable closure ONLY for internal functions, but for
userland functions binding should be still possible, right?

E.g.

class Foo {private $a=1;}

function test() {
var_dump($this);
}
$closure = (new \ReflectionFunction('test'))->getClosure();
$object = new Foo;
$closure = $closure->bindTo($object, get_class($object));
$closure();

(unfortunately, it's broken: https://3v4l.org/YeDBj#v700rc5)

I know, that binding functions to objects is tricky, but this can be useful
for utility classes. But if you insist on this, ok, it's very rare case,
just one single example.

What is more important for me is ability to work with methods as closures.
So my main concern is about restrictions of ReflectionMethod->getClosure()
and rebinding.

See this example:

class Foo {private $a=1;}

class Bar {
function test() {
var_dump($this);
}
}
$closure = (new \ReflectionMethod('Bar','test'))->getClosure(new Bar);
$object = new Foo;
$closure = $closure->bindTo($object, get_class($object));
$closure();

This kind of rebinding is heavily used by Go! Aspect-Oriented Framework and
now it will be broken (see https://3v4l.org/cZtbI#v700rc5). I think there
will be some more frameworks/libraries that use reflection and closure
rebinding via reflections. So this introduced patch for RC5 is definitely a
BC break for existing code.
Could you please provide a workaround or alternative ways to keep it
working? Maybe, restrict rebinding only for internal objects/methods will
be enough? Or only for ReflectionFunctionAbstract->getClosure(), but not
for ReflectionMethod->getClosure()

Thanks!

2015-10-13 10:27 GMT+03:00 Anatol Belski anatol.php@belski.net:

Hi,

-----Original Message-----
From: Xinchen Hui [mailto:laruence@php.net]
Sent: Tuesday, October 13, 2015 4:23 AM
To: Anatol Belski anatol.php@belski.net
Cc: Nikita Popov nikita.ppv@gmail.com; Dmitry Stogov <dmitry@zend.com
;
PHP internals internals@lists.php.net
Subject: Re: [PHP-DEV] Re: Forbid rebinding scope of closures created by
ReflectionFunctionAbstract::getClosure()

Hey:

On Tue, Oct 13, 2015 at 6:12 AM, Anatol Belski anatol.php@belski.net
wrote:

-----Original Message-----
From: Nikita Popov [mailto:nikita.ppv@gmail.com]
Sent: Monday, October 12, 2015 10:33 PM
To: Anatol Belski anatol.php@belski.net
Cc: Dmitry Stogov dmitry@zend.com; PHP internals <
internals@lists.php.net>
Subject: Re: [PHP-DEV] Re: Forbid rebinding scope of closures
created by
ReflectionFunctionAbstract::getClosure()

On Mon, Oct 12, 2015 at 10:22 PM, Anatol Belski
anatol.php@belski.net
wrote:

Hi,

-----Original Message-----
From: Nikita Popov [mailto:nikita.ppv@gmail.com]
Sent: Monday, October 12, 2015 8:57 PM
To: Dmitry Stogov dmitry@zend.com
Cc: PHP internals internals@lists.php.net; Andrea Faulds
ajf@ajf.me;
Stas
Malyshev smalyshev@sugarcrm.com; Bob Weinand
bwoebi@php.net;
Anatol Belski anatol.php@belski.net
Subject: [PHP-DEV] Re: Forbid rebinding scope of closures
created by
ReflectionFunctionAbstract::getClosure()

It would be great, if we stop any commits into PHP-7.0 except
for
critical fixes now

Maybe keep PHP-7.0 open (or as open as release branches usually
are),
but from
now on only cherry-pick critical fixes into PHP-7.0.0 (instead
of merging everything)?

I commit myself to Dmitry's words. What matters today and
especially after
RC5 is the stability. Today we should invest into testing and bug
fixes more than into improvements (aka fixes to something that is
not broken). It really matters for the quality of the final.
That's the message to convey probably.

To rephrase my question: Should we treat PHP-7.0 the same way we
treat
PHP-5.6 and other release branches (i.e. all kinds of bug fixes are
okay, even if
it's just improving a test or fixing some inconsequential behavior),
or
do you
want to limit the PHP-7.0 branch to actually critical fixes now?
From
what you
say, I assume the former rather than the latter?

Talking about "critical" I meant usual definitions as something that
causes memory corruptions, data loss, big functional impact, security
flaws, etc.

agree, bugs

The tricky point with 7.0 right now is that it's a lot of new stuff
with very high expectations standing short before final. Coupling this
with the "critical" from above, the definition I would make were - it
is a) critical and b) can be fixed properly and cannot wait until
after the final
release.
The dev time spent to fix something after 7.0 final is indeed lost for
the
7.0 final. Any new code short before final increases the instability
risk of the final.

Fe small functional regressions are probably not always critical. If a
(even small) functional regression breaks a lot, it is critical. If a
functional regression breaks a rare use case - it is not critical. If
improving a test helps to cover some critical code - so yes, it is
critical as well. Anything that is critical can involve anything
you've mentioned like adding tests or code.

hmm, I understand your ideas, but I think maybe we should forbid such
things as
well for this moment, because it make things complicated to judge(if it
is critical
or not), such things can goes to 7.1

let's only allows bugs fix (as you mentioned above) to be committed into
PHP-7.0 before the final release of its

Yeah, such things are quite stretchable, as we don't really assign tickets
to milestones. Concentrating on hard weight things only were probably the
best strategy, as you say. I'll be mentioning this yet in the next
announcement as well.

Regards

anatol