Skip to content

Fix inconsistent $this behavior - #1918

Closed
dstogov wants to merge 13 commits into
php:masterfrom
dstogov:this_var
Closed

Fix inconsistent $this behavior#1918
dstogov wants to merge 13 commits into
php:masterfrom
dstogov:this_var

Conversation

@dstogov

Copy link
Copy Markdown
Member

No description provided.

@dstogovdstogov changed the title Fixed inconsistent $this behaviorFix inconsistent $this behaviorMay 23, 2016
Comment threadext/standard/array.c
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")) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to use zend_string_equals(Z_STR(final_name), CG(known_string)[THIS_STR[)

* master:
Fixed bug #72213 (Finally leaks on nested exceptions).
Forbid dynamic calls to scope introspection functions
Allow empty property names
Ensure no entry predecessors for SSA construction
Replace BB end with BB len
Fixed white-spaces
@nikic

Copy link
Copy Markdown
Member

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?)

@dstogov

Copy link
Copy Markdown
MemberAuthor

@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.

@nikic

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
MemberAuthor

ah, right. I'll think about this.


From: Nikita Popov notifications@github.com
Sent: Thursday, May 26, 2016 2:01:01 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Fix inconsistent $this behavior (#1918)

@dstogovhttps://github.com/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.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1918#issuecomment-221732782

@nikic

Copy link
Copy Markdown
Member

@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.

dstogov added 2 commits May 31, 2016 11:03
* master: (45 commits)
Re-Fixed bug #72155 (use-after-free caused by get_zval_xmlrpc_type)
Revert "fix #72155 (use-after-free caused by get_zval_xmlrpc_type)"
Split ZEND_SEND_VAR_NO_REF into ZEND_SEND_VAR_NO_REF and ZEND_SEND_VAR_NO_REF_EX (similar to ZEND_SEND_VAL) and remove ZEND_ARG_* flags.
Initialize only the necessary fields.
fix condition
update NEWS
Fixed bug #72284 (phpdbg fatal errors with coverage)
fix test title
Add test for bug #72258
update UPGRADING
Expose missing flags from libzip at least >= 0.11.x
update UPGRADING
Expose missing flags from libzip at least >= 0.11.x
fix #72155 (use-after-free caused by get_zval_xmlrpc_type)
This is exported at implementation site, but no forward declaration can cause compile warnings
Fix bug #71604
Forbid "yield from" in force closed generators
Added NEWS Entry
Test for bug #72221, segfault in zend_memnstr_ex
Fix bug #72221 (segfault, past-the-end access)
...
@dstogov

Copy link
Copy Markdown
MemberAuthor

@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.

@ml-

ml- commented May 31, 2016

Copy link
Copy Markdown
<?phperror_reporting(E_ALL);
class Foo {
function__construct() {
$this->bar($this);
var_dump($this);
}
publicfunctionbar(&$that) {
$that = "Bar";
}
}
$q = newFoo();
master
string(3) "Bar"
dstogov/this_var
object(Foo)#1 (0) {
}

Works.
However, no Warning emitted or Exception thrown in this case.

@dstogov

Copy link
Copy Markdown
MemberAuthor

Right. This case is covered by corresponding RFC section https://wiki.php.net/rfc/this_var#disable_ability_to_re-assign_this_indirectly_through_reference


From: Matthias Lisin notifications@github.com
Sent: Tuesday, May 31, 2016 6:55:52 PM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Fix inconsistent $this behavior (#1918)

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. ## You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com//pull/1918#issuecomment-222733471, or mute the threadhttps://github.com/notifications/unsubscribe/ACZM0hZB4bDoLS4OP4oL9SYc24pT2budks5qHFoIgaJpZM4Iky76.

@staabm

staabm commented May 31, 2016

Copy link
Copy Markdown
Contributor

Should it also emit a warning/notice in this case?

} else {
ssa_var_info[i].type = MAY_BE_UNDEF | MAY_BE_RCN;
}
ssa_var_info[i].type = MAY_BE_UNDEF | MAY_BE_RCN;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should add inference for the new FETCH_THIS opcodes.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Majkl578

Majkl578 commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

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

* master: (134 commits)
These bugs are also in 7.1-alpha
Fixed(attempt to) bug #72405 (mb_ereg_replace - mbc_to_code (oniguruma) - oob read access)
Maybe fix bug #72011
Fix #50845: exif_process_IFD_TAG: Use the right offset if reading from stream
Improve the signature
Unused var
C89 compatibility
Fix bug #72138 - Integer Overflow in Length of String-typed ZVAL
Only allow single comma in tail
Fixed bug #72399 (Use-After-Free in MBString (search_re))
Implemented FR #72385 (Update SQLite bundle lib(3.13.0))
Cleanup
Add support for "instanceof" pi nodes
Use union for pi constraints
Cleanup
Fixed bug #72395 (list() regression)
Switch zend_print_zval_r to use smart_str
fix test portability
Fixed bug #72306 (Heap overflow through proc_open and $env parameter)
update NEWS
...
It throws exception if not in object context.
Removed useless opcode handlers.
Comment threadZend/zend_vm_def.h
USE_OPLINE
zval *result = EX_VAR(opline->result.var);

ZVAL_BOOL(EX_VAR(opline->result.var),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use result (or the result variable can be dropped instead).

* master:
Updated to version 2016.5 (2016e)
Updated to version 2016.5 (2016e)
Updated to version 2016.5 (2016e)
* master:
Fix type inference bugs
Added specialized handlers for SEND_VAR/SEND_VAR_EX opcodes.
Fixed mistakes in type inference rules.
Fixed expected test outcome due to rule changes
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@dstogov@nikic@ml-@staabm@Majkl578@laruence