Skip to content

Commit

Permalink
Relax closure $this unbinding deprecation
Browse files Browse the repository at this point in the history
Only deprecate unbinding of $this from a closure if $this is
syntactically used within the closure.

This is desired to support Laravel's macro system, see laravel/framework#29482.

This should still allow us to implement the performance improvements
we're interested in for PHP 8, without breaking existing use-cases.
  • Loading branch information
nikic committed Aug 23, 2019
1 parent 8807889 commit d1157cb
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 2 deletions.
2 changes: 1 addition & 1 deletion UPGRADING
Expand Up @@ -365,7 +365,7 @@ PHP 7.4 UPGRADE NOTES
ReflectionMethod::getClosure() and closure rebinding is deprecated. Doing
so is equivalent to calling a non-static method statically, which has been
deprecated since PHP 7.0.
. Unbinding $this of a non-static closure is deprecated.
. Unbinding $this of a non-static closure that uses $this is deprecated.
. Using "parent" inside a class without a parent is deprecated, and will throw
a compile-time error in the future. Currently an error will only be
generated if/when the parent is accessed at run-time.
Expand Down
56 changes: 56 additions & 0 deletions Zend/tests/closure_062.phpt
@@ -0,0 +1,56 @@
--TEST--
Closure $this unbinding deprecation
--FILE--
<?php

class Test {
public function method() {
echo "instance scoped, non-static, \$this used\n";
$fn = function() {
var_dump($this);
};
$fn->bindTo(null);
echo "instance scoped, static, \$this used\n";
$fn = static function() {
var_dump($this);
};
$fn->bindTo(null);
echo "instance scoped, non-static, \$this not used\n";
$fn = function() {
var_dump($notThis);
};
$fn->bindTo(null);
}

public static function staticMethod() {
echo "static scoped, non-static, \$this used\n";
$fn = function() {
var_dump($this);
};
$fn->bindTo(null);
echo "static scoped, static, \$this used\n";
$fn = static function() {
var_dump($this);
};
$fn->bindTo(null);
echo "static scoped, static, \$this not used\n";
$fn = function() {
var_dump($notThis);
};
$fn->bindTo(null);
}
}

(new Test)->method();
Test::staticMethod();

?>
--EXPECTF--
instance scoped, non-static, $this used

Deprecated: Unbinding $this of closure is deprecated in %s on line %d
instance scoped, static, $this used
instance scoped, non-static, $this not used
static scoped, non-static, $this used
static scoped, static, $this used
static scoped, static, $this not used
3 changes: 2 additions & 1 deletion Zend/zend_closures.c
Expand Up @@ -90,7 +90,8 @@ static zend_bool zend_valid_closure_binding(
} else {
zend_error(E_DEPRECATED, "Unbinding $this of a method is deprecated");
}
} else if (!is_fake_closure && !Z_ISUNDEF(closure->this_ptr)) {
} else if (!is_fake_closure && !Z_ISUNDEF(closure->this_ptr)
&& (func->common.fn_flags & ZEND_ACC_USES_THIS)) {
// TODO: Only deprecate if it had $this *originally*?

This comment has been minimized.

Copy link
@kocsismate

kocsismate Aug 23, 2019

Member

@nikic This TODO comment could be removed, couldn't it?

This comment has been minimized.

Copy link
@nikic

nikic Aug 25, 2019

Author Member

Not quite, you could still have a freestanding close (no $this scope) that uses $this and then bind $this and then unbind $this again, which would currently generate a deprecation, but possibly shouldn't.

This comment has been minimized.

Copy link
@kocsismate

kocsismate Aug 26, 2019

Member

Uh, thank you very much for the background information :) This change seemed straightforward, but as it seems, it wasn't - at least for me.

zend_error(E_DEPRECATED, "Unbinding $this of closure is deprecated");
}
Expand Down
5 changes: 5 additions & 0 deletions Zend/zend_compile.c
Expand Up @@ -2380,6 +2380,7 @@ static zend_op *zend_compile_simple_var(znode *result, zend_ast *ast, uint32_t t
opline->result_type = IS_TMP_VAR;
result->op_type = IS_TMP_VAR;
}
CG(active_op_array)->fn_flags |= ZEND_ACC_USES_THIS;
return opline;
} else if (zend_try_compile_cv(result, ast) == FAILURE) {
return zend_compile_simple_var_no_cv(result, ast, type, delayed);
Expand Down Expand Up @@ -2474,6 +2475,7 @@ static zend_op *zend_delayed_compile_prop(znode *result, zend_ast *ast, uint32_t

if (is_this_fetch(obj_ast)) {
obj_node.op_type = IS_UNUSED;
CG(active_op_array)->fn_flags |= ZEND_ACC_USES_THIS;
} else {
opline = zend_delayed_compile_var(&obj_node, obj_ast, type, 0);
if (opline && type == BP_VAR_W && (opline->opcode == ZEND_FETCH_STATIC_PROP_W || opline->opcode == ZEND_FETCH_OBJ_W)) {
Expand Down Expand Up @@ -3010,6 +3012,7 @@ uint32_t zend_compile_args(zend_ast *ast, zend_function *fbc) /* {{{ */
if (is_this_fetch(arg)) {
zend_emit_op(&arg_node, ZEND_FETCH_THIS, NULL, NULL);
opcode = ZEND_SEND_VAR_EX;
CG(active_op_array)->fn_flags |= ZEND_ACC_USES_THIS;
break;
} else if (zend_try_compile_cv(&arg_node, arg) == SUCCESS) {
opcode = ZEND_SEND_VAR_EX;
Expand Down Expand Up @@ -3878,6 +3881,7 @@ void zend_compile_method_call(znode *result, zend_ast *ast, uint32_t type) /* {{

if (is_this_fetch(obj_ast)) {
obj_node.op_type = IS_UNUSED;
CG(active_op_array)->fn_flags |= ZEND_ACC_USES_THIS;
} else {
zend_compile_expr(&obj_node, obj_ast);
}
Expand Down Expand Up @@ -7785,6 +7789,7 @@ void zend_compile_isset_or_empty(znode *result, zend_ast *ast) /* {{{ */
case ZEND_AST_VAR:
if (is_this_fetch(var_ast)) {
opline = zend_emit_op(result, ZEND_ISSET_ISEMPTY_THIS, NULL, NULL);
CG(active_op_array)->fn_flags |= ZEND_ACC_USES_THIS;
} else if (zend_try_compile_cv(&var_node, var_ast) == SUCCESS) {
opline = zend_emit_op(result, ZEND_ISSET_ISEMPTY_CV, &var_node, NULL);
} else {
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_compile.h
Expand Up @@ -337,6 +337,9 @@ typedef struct _zend_oparray_context {
/* function is a destructor | | | */
#define ZEND_ACC_DTOR (1 << 29) /* | X | | */
/* | | | */
/* closure uses $this | | | */
#define ZEND_ACC_USES_THIS (1 << 30) /* | X | | */
/* | | | */
/* op_array uses strict mode types | | | */
#define ZEND_ACC_STRICT_TYPES (1U << 31) /* | X | | */

Expand Down

0 comments on commit d1157cb

Please sign in to comment.