Skip to content

Fix #79487: ::getStaticProperties() ignores property modifications #5429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
cmb69 wants to merge 5 commits into from
Closed

Fix #79487: ::getStaticProperties() ignores property modifications #5429

cmb69 wants to merge 5 commits into from

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Apr 21, 2020

When retrieving the static class properties via reflection, we have to
cater to possible modifications.

@cmb69
Fix #79487: ::getStaticProperties() ignores property modifications
ad2ab3a 
When retrieving the static class properties via reflection, we have to
cater to possible modifications.
@cmb69 cmb69 added the Bug label Apr 21, 2020
nikic
nikic reviewed Apr 21, 2020
View reviewed changes
@cmb69
Inline special cased add_class_vars()
2156b98 
As suggested by Nikita.
nikic
nikic reviewed Apr 21, 2020
View reviewed changes
nikic
nikic reviewed Apr 21, 2020
View reviewed changes
nikic
nikic reviewed Apr 21, 2020
View reviewed changes
@cmb69
Copy link
Member Author

cmb69 commented Apr 21, 2020

Thanks @nikic! I'll have a closer look at this ASAP.

@nikic
Copy link
Member

nikic commented May 19, 2020

Ping

@cmb69
Copy link
Member Author

cmb69 commented May 19, 2020

Sorry for the delay.

@cmb69
Copy link
Member Author

cmb69 commented Jun 23, 2020

Pong :)

@nikic
Copy link
Member

nikic commented Jun 23, 2020

@cmb69

<?php
  
class A {
    public static $a = 'A old';
}
class B extends A {
    public static $b = 'B old';
}

$rc = new ReflectionClass(B::class);
A::$a = 'A new';
var_dump($rc->getStaticProperties());

Incorrectly prints under opcache:

array(2) {
  ["b"]=>
  string(5) "B old"
  ["a"]=>
  string(5) "A old"
}

@cmb69
Init class statics if necessary
237b1b9 
This is necessary for inherited properties, and allows us to drop the
check later.

We also remove the unnecessary `IS_CONSTANT_AST` handling.
nikic
nikic reviewed Jun 23, 2020
View reviewed changes
ext/reflection/php_reflection.c

/* copy: enforce read only access */
ZVAL_DEREF(prop);
ZVAL_COPY_OR_DUP(&prop_copy, prop);
Copy link
Member

@nikic nikic Jun 23, 2020

Choose a reason for hiding this comment

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

The CE_STATIC_MEMBERS is already per-request, so it's not necessary to COPY_OR_DUP. Just doing a Z_TRY_ADDREF_P(prop) is sufficient here.

Copy link
Member Author

@cmb69 cmb69 Jun 23, 2020

Choose a reason for hiding this comment

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

Fine! :)

nikic
nikic approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LG apart from the comment.

@cmb69
Copy link
Member Author

cmb69 commented Jun 23, 2020

Thanks! Applied as a895bb6.

@cmb69 cmb69 closed this Jun 23, 2020
@cmb69 cmb69 deleted the cmb/79487 branch June 23, 2020 16:54
@cmb69
Copy link
Member Author

cmb69 commented Jun 24, 2020

FTR: I had to revert a895bb6 due to segfaults in mysqli tests. Amended PR now applied as f3cccfd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cmb69 @nikic