Skip to content

Fix #74922 - Try to resolve constants when importing trait properties #2779

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
wants to merge 1 commit into from
Closed

Fix #74922 - Try to resolve constants when importing trait properties #2779

wants to merge 1 commit into from

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Sep 27, 2017

Link for bugsnet: https://bugs.php.net/bug.php?id=74922

It fixes the reported issue:

const VALUE = true;

trait Foo {public $var = VALUE;}
trait Bar {public $var = VALUE;}
class Baz {use Foo, Bar;}

And it also allows for values that are identical to the constant like:

const VALUE = true;

trait Foo {public $var = VALUE;}
trait Bar {public $var = true;}
class Baz {use Foo, Bar;}

@krakjoe krakjoe added the Bug label Sep 27, 2017
nikic
nikic reviewed Sep 27, 2017
View reviewed changes
Zend/zend_inheritance.c Outdated
}
if (UNEXPECTED(Z_TYPE_P(op2) == IS_CONSTANT)) {
op2 = zend_get_constant_ex(Z_STR_P(op2), ce, ZEND_FETCH_CLASS_SILENT);
}
Copy link
Member

Choose a reason for hiding this comment

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

This should use zend_update_constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks for the feedback

@nikic
Copy link
Member

nikic commented Oct 28, 2017

Edge-case:

trait T {
    public $x = self::X;
}
trait T2 {
    public $x = self::X;
}
class C {
    use T, T2;
    const X = 42;
}
var_dump((new C)->x);

Gives:

Warning: Uncaught Error: Cannot access self:: when no class scope is active in /home/nikic/php-src/t084.php:9
Stack trace:
#0 {main}

Next Error: Cannot access self:: when no class scope is active in /home/nikic/php-src/t084.php:9
Stack trace:
#0 {main}
  thrown in /home/nikic/php-src/t084.php on line 9

Fatal error: T and T2 define the same property ($x) in the composition of C. However, the definition differs and is considered incompatible. Class was composed in /home/nikic/php-src/t084.php on line 9

@pmmaga
Copy link
Contributor Author

pmmaga commented Oct 28, 2017

@nikic, nice find. I've updated the usage of zval_update_constant to zval_update_constant_ex which fixes the issue. I've also added your example as a test.

@nikic
Copy link
Member

nikic commented Oct 28, 2017

Slight extension:

<?php

trait T {
    public $x = self::X;
}
trait T2 {
    public $x = self::X;
}
class C {
    use T, T2;
    const X = 42;
}
class D {
    use T2;
    const X = 24;
}
var_dump((new C)->x);
var_dump((new D)->x);

This gives 42 42 instead of 42 24. The zend_update_constants_ex shouldn't be performed in-place, but rather on a copy of the zvals.

@pmmaga
Copy link
Contributor Author

pmmaga commented Oct 28, 2017

Updated so that the constant resolution happens on copies.

@nikic
Copy link
Member

nikic commented Nov 2, 2017

Merged as 897bdb4 with a few tweaks (using COPY_OR_DUP and adding dtor calls). Thanks!

@nikic nikic closed this Nov 2, 2017
@pmmaga pmmaga mentioned this pull request Dec 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pmmaga @nikic @krakjoe