Skip to content

Fix #[Override] on traits overriding a parent method without a matching interface #12205

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.

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

Merged
merged 3 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ PHP NEWS
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
?? ??? ????, PHP 8.3.0RC3

- Core:
. Fixed bug GH-12189 (#[Override] attribute in trait does not check for
parent class implementations). (timwolla)

- Filter:
. Fix explicit FILTER_REQUIRE_SCALAR with FILTER_CALLBACK (ilutov)

Expand Down
42 changes: 42 additions & 0 deletions Zend/tests/attributes/override/gh12189.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
--TEST--
#[Override] attribute in trait does not check for parent class implementations
--FILE--
<?php

class A {
public function foo(): void {}
}

interface I {
public function foo(): void;
}

trait T {
#[\Override]
public function foo(): void {
echo 'foo';
}
}

// Works fine
class B implements I {
use T;
}

// Works fine ("copied and pasted into the target class")
class C extends A {
#[\Override]
public function foo(): void {
echo 'foo';
}
}

// Does not work
class D extends A {
use T;
}
echo "Done";

?>
--EXPECT--
Done
24 changes: 24 additions & 0 deletions Zend/tests/attributes/override/gh12189_2.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
#[Override] attribute in trait does not check for parent class implementations (Variant with private parent method)
--FILE--
<?php

class A {
private function foo(): void {}
}

trait T {
#[\Override]
public function foo(): void {
echo 'foo';
}
}

class D extends A {
use T;
}
echo "Done";

?>
--EXPECTF--
Fatal error: D::foo() has #[\Override] attribute, but no matching parent method exists in %s on line %d
24 changes: 24 additions & 0 deletions Zend/tests/attributes/override/gh12189_3.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
#[Override] attribute in trait does not check for parent class implementations (Variant with protected parent method)
--FILE--
<?php

class A {
protected function foo(): void {}
}

trait T {
#[\Override]
public function foo(): void {
echo 'foo';
}
}

class D extends A {
use T;
}
echo "Done";

?>
--EXPECTF--
Done
24 changes: 24 additions & 0 deletions Zend/tests/attributes/override/gh12189_4.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
#[Override] attribute in trait does not check for parent class implementations (Variant with __construct)
--FILE--
<?php

class A {
public function __construct() {}
}

trait T {
#[\Override]
public function __construct() {
echo 'foo';
}
}

class D extends A {
use T;
}
echo "Done";

?>
--EXPECTF--
Fatal error: D::__construct() has #[\Override] attribute, but no matching parent method exists in %s on line %d
24 changes: 24 additions & 0 deletions Zend/tests/attributes/override/gh12189_5.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
#[Override] attribute in trait does not check for parent class implementations (Variant with abstract __construct)
--FILE--
<?php

abstract class A {
abstract public function __construct();
}

trait T {
#[\Override]
public function __construct() {
echo 'foo';
}
}

class D extends A {
use T;
}
echo "Done";

?>
--EXPECT--
Done
25 changes: 25 additions & 0 deletions Zend/tests/attributes/override/gh12189_6.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
--TEST--
#[Override]: Inheritance check of inherited trait method against interface
--FILE--
<?php

trait T1 {
public abstract function test();
}

trait T2 {
public function test() {}
}

class A {
use T2;
}

class B extends A {
use T1;
}

?>
===DONE===
--EXPECT--
===DONE===
26 changes: 16 additions & 10 deletions Zend/zend_inheritance.c
Original file line number Diff line number Diff line change
Expand Up @@ -1081,12 +1081,13 @@ static void perform_delayable_implementation_check(
/**
* @param check_only Set to false to throw compile errors on incompatible methods, or true to return INHERITANCE_ERROR.
* @param checked Whether the compatibility check has already succeeded in zend_can_early_bind().
* @param force_mutable Whether we know that child may be modified, i.e. doesn't live in shm.
*/
static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(
zend_function *child, zend_class_entry *child_scope,
zend_function *parent, zend_class_entry *parent_scope,
zend_class_entry *ce, zval *child_zv,
bool check_visibility, bool check_only, bool checked) /* {{{ */
bool check_visibility, bool check_only, bool checked, bool force_mutable) /* {{{ */
{
uint32_t child_flags;
uint32_t parent_flags = parent->common.fn_flags;
Expand Down Expand Up @@ -1188,7 +1189,7 @@ static zend_always_inline inheritance_status do_inheritance_check_on_method_ex(
perform_delayable_implementation_check(ce, child, child_scope, parent, parent_scope);
}

if (!check_only && child->common.scope == ce) {
if (!check_only && (child->common.scope == ce || force_mutable)) {
child->common.fn_flags &= ~ZEND_ACC_OVERRIDE;
}

Expand All @@ -1201,7 +1202,7 @@ static zend_never_inline void do_inheritance_check_on_method(
zend_function *parent, zend_class_entry *parent_scope,
zend_class_entry *ce, zval *child_zv, bool check_visibility)
{
do_inheritance_check_on_method_ex(child, child_scope, parent, parent_scope, ce, child_zv, check_visibility, 0, 0);
do_inheritance_check_on_method_ex(child, child_scope, parent, parent_scope, ce, child_zv, check_visibility, 0, 0, /* force_mutable */ false);
}

static zend_always_inline void do_inherit_method(zend_string *key, zend_function *parent, zend_class_entry *ce, bool is_interface, bool checked) /* {{{ */
Expand All @@ -1219,7 +1220,7 @@ static zend_always_inline void do_inherit_method(zend_string *key, zend_function
if (checked) {
do_inheritance_check_on_method_ex(
func, func->common.scope, parent, parent->common.scope, ce, child,
/* check_visibility */ 1, 0, checked);
/* check_visibility */ 1, 0, checked, /* force_mutable */ false);
} else {
do_inheritance_check_on_method(
func, func->common.scope, parent, parent->common.scope, ce, child,
Expand Down Expand Up @@ -1946,6 +1947,7 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_
{
zend_function *existing_fn = NULL;
zend_function *new_fn;
bool check_inheritance = false;

if ((existing_fn = zend_hash_find_ptr(&ce->function_table, key)) != NULL) {
/* if it is the same function with the same visibility and has not been assigned a class scope yet, regardless
Expand Down Expand Up @@ -1980,11 +1982,7 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_
ZSTR_VAL(ce->name), ZSTR_VAL(name),
ZSTR_VAL(existing_fn->common.scope->name), ZSTR_VAL(existing_fn->common.function_name));
} else {
/* Inherited members are overridden by members inserted by traits.
* Check whether the trait method fulfills the inheritance requirements. */
do_inheritance_check_on_method(
fn, fixup_trait_scope(fn, ce), existing_fn, fixup_trait_scope(existing_fn, ce),
ce, NULL, /* check_visibility */ 1);
check_inheritance = true;
}
}

Expand All @@ -2004,6 +2002,14 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_
function_add_ref(new_fn);
fn = zend_hash_update_ptr(&ce->function_table, key, new_fn);
zend_add_magic_method(ce, fn, key);

if (check_inheritance) {
/* Inherited members are overridden by members inserted by traits.
* Check whether the trait method fulfills the inheritance requirements. */
do_inheritance_check_on_method_ex(
fn, fixup_trait_scope(fn, ce), existing_fn, fixup_trait_scope(existing_fn, ce),
ce, NULL, /* check_visibility */ 1, false, false, /* force_mutable */ true);
}
}
/* }}} */

Expand Down Expand Up @@ -3239,7 +3245,7 @@ static inheritance_status zend_can_early_bind(zend_class_entry *ce, zend_class_e
do_inheritance_check_on_method_ex(
child_func, child_func->common.scope,
parent_func, parent_func->common.scope,
ce, NULL, /* check_visibility */ 1, 1, 0);
ce, NULL, /* check_visibility */ 1, 1, 0, /* force_mutable */ false);
if (UNEXPECTED(status == INHERITANCE_WARNING)) {
overall_status = INHERITANCE_WARNING;
} else if (UNEXPECTED(status != INHERITANCE_SUCCESS)) {
Expand Down