Fix inconsistent $this behavior #1918
6 participants
dstogov
changed the title from Fixed inconsistent $this behavior to Fix inconsistent $this behavior 
| @@ -1889,6 +1884,15 @@ PHP_FUNCTION(extract) | ||
| if (Z_TYPE(final_name) == IS_STRING && php_valid_var_name(Z_STRVAL(final_name), Z_STRLEN(final_name))) { | ||
| zval *orig_var; | ||
| + | ||
| + if (Z_STRLEN(final_name) == sizeof("this")-1 && !strcmp(Z_STRVAL(final_name), "this")) { |
|
it's better to use zend_string_equals(Z_STR(final_name), CG(known_string)[THIS_STR[)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
From a quick scan, it looks like handling for by-reference fetches of $this is missing. A by-ref fetch should not throw the undefined variable notice if $this does not exist. (And for function arguments by-ref may be dynamic, so do we also need a FUNC_ARG variant of this?)
@nikic it's possible to pass or assign $this by reference, but this assignment is actually done by value and changes to reference won't affect value of $this.
@dstogov Yeah, I did understand that. What I'm referring to is that reference assignment usually suppresses the "undefined variable" notice, but in this case it would still be there, right? Though probably this doesn't matter anyway.
@dstogov Tangentially related, as it would solve the issue with references as well: Shouldn't any access to $this, apart from isset($this), throw the same error exception you get on $this->foo and similar? It seems inconsistent that $this->foo; and $that = $this; $that->foo; behave very differently if $this is not defined. In one case you get an error exception, in the other you get a notice and warning. As long as isset($this) does not throw, this still keeps BC with the "can use method statically and non-statically" pattern.
@nikic, good point. I followed your suggestion and changed RFC and implementation accordingly. Now we will get "Using $this when not in object context" exception everywhere.
<?php
error_reporting(E_ALL);
class Foo {
function __construct() {
$this->bar($this);
var_dump($this);
}
public function bar(&$that) {
$that = "Bar";
}
}
$q = new Foo();master
string(3) "Bar"
dstogov/this_var
object(Foo)#1 (0) {
}
Works.
However, no Warning emitted or Exception thrown in this case.
| @@ -3844,13 +3843,7 @@ int zend_ssa_inference(zend_arena **arena, const zend_op_array *op_array, const | ||
| } | ||
| } else { | ||
| for (i = 0; i < op_array->last_var; i++) { | ||
| - if (i == EX_VAR_TO_NUM(op_array->this_var)) { | ||
| - ssa_var_info[i].type = MAY_BE_UNDEF | MAY_BE_RC1 | MAY_BE_RCN | MAY_BE_OBJECT; | ||
| - ssa_var_info[i].ce = op_array->scope; | ||
| - ssa_var_info[i].is_instanceof = 1; | ||
| - } else { | ||
| - ssa_var_info[i].type = MAY_BE_UNDEF | MAY_BE_RCN; | ||
| - } | ||
| + ssa_var_info[i].type = MAY_BE_UNDEF | MAY_BE_RCN; |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| @@ -4614,6 +4614,9 @@ PHP_FUNCTION(parse_str) | ||
| symbol_table = zend_rebuild_symbol_table(); | ||
| ZVAL_ARR(&tmp, symbol_table); | ||
| sapi_module.treat_data(PARSE_STRING, res, &tmp); | ||
| + if (UNEXPECTED(zend_hash_del(symbol_table, CG(known_strings)[ZEND_STR_THIS]) == SUCCESS)) { | ||
| + zend_throw_error(NULL, "Cannot re-assign $this"); | ||
| + } |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
nikic
and 1 other
commented on an outdated diff
| + } | ||
| +} | ||
| + | ||
| +ZEND_VM_HANDLER(185, ZEND_FETCH_THIS_IS, UNUSED, UNUSED) | ||
| +{ | ||
| + USE_OPLINE | ||
| + zval *result = EX_VAR(opline->result.var); | ||
| + | ||
| + if (EXPECTED(Z_TYPE(EX(This)) == IS_OBJECT)) { | ||
| + ZVAL_OBJ(result, Z_OBJ(EX(This))); | ||
| + Z_ADDREF_P(result); | ||
| + } else { | ||
| + ZVAL_NULL(result); | ||
| + } | ||
| + ZEND_VM_NEXT_OPCODE(); | ||
| +} |
|
This opcode comes up only for Right now
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| @@ -0,0 +1,11 @@ | ||
| +--TEST-- | ||
| +$this in foreach | ||
| +--FILE-- | ||
| +<?php | ||
| +$a = [1]; | ||
| +foreach ($a as $this) { | ||
| + var_dump($this); | ||
| +} | ||
| +?> | ||
| +--EXPECTF-- | ||
| +Fatal error: Cannot re-assign $this in %sthis_in_foreach_001.php on line 3 |
|
This message is a bit strange. Since there is no
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
This RFC targets 7.1 although introduces mutliple BC breaks. It seems like a violation of PHP's own release process:
x.y.z to x.y+1.z
- Backward compatibility must be kept
nikic
commented on an outdated diff
| + ZVAL_OBJ(result, Z_OBJ(EX(This))); | ||
| + Z_ADDREF_P(result); | ||
| + ZEND_VM_NEXT_OPCODE(); | ||
| + } else { | ||
| + SAVE_OPLINE(); | ||
| + zend_throw_error(NULL, "Using $this when not in object context"); | ||
| + HANDLE_EXCEPTION(); | ||
| + } | ||
| +} | ||
| + | ||
| +ZEND_VM_HANDLER(186, ZEND_ISSET_ISEMPTY_THIS, UNUSED, UNUSED) | ||
| +{ | ||
| + USE_OPLINE | ||
| + zval *result = EX_VAR(opline->result.var); | ||
| + | ||
| + ZVAL_BOOL(EX_VAR(opline->result.var), |
|
This should probably use
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
No description provided.