Warn if a parent method with TrustedCallback attribute is extended

[FatherShawn]

Feature request

Annotations in PHP are not inherited

Does m in the child inherit the attributes from m in the parent? What about if it's protected, or private? You could say to follow the normal rules of class inheritance, but with complex object trees, those can get tricky pretty quickly, and it's probably not worth it for a feature that's meant to be purely static (more on that later).

But ultimately: manually adding a new attribute is easy, but removing an automatically inherited one would be complicated. I think it was the right call by the devs.

Can we have a rule that checks if a method is overriding a parent method that is annotated with TrustedCallback?

See https://www.drupal.org/project/drupal/issues/3354584#comment-15260556

[mglaman]

Im confused. If we are checking that, aren't we breaking expected behavior? A method overriding a parent doesn't automatically get that trait. Or am I reading this wrong.

[FatherShawn]

Correct, the attribute applies only to the method where it appears. I guess I need to find an example in core where this could be a problem first. Here's a simplified case:

class base {

use Drupal\Core\Security\Attribute\TrustedCallback;

  #[TrustedCallback]
  public function foo() {
    // Do something.
  }

}

class child extends base {

  public function foo() {
    // Do something different.
  }

}

When we used an Interface approach, child would inherit the interface from base as well as the trustedCallbacks method that declared foo to be trusted. Nothing needs to be added to child for child::foo() to be trusted. If we deprecate the interface for the attribute because it's simpler to maintain, then we loose that. Someone extending base needs to add the attribute annotation to foo if they override the method.

It seems useful to raise the issue - is omitting the attribute intended?

[mglaman]

I guess my concern is if Drupal respects this? If PHP doesn't then it shouldn't work. Or are Drupal tests passing somehow because we added checks manually for parent methods?

If Drupal manually does that check, then it makes total sense for this to as well

[mglaman]

if (!$safe_callback) { 

$method = new \ReflectionMethod($object_or_classname, $method_name); 

$safe_callback = (bool) $method->getAttributes(TrustedCallback::class); 

}

That's the code from Drupal. It doesn't do anything fancy around inheritance.

So it seems like part of the fix is still required in Drupal core as well

[FatherShawn]

Leaving this here as a reminder, and continuing the discussion at https://www.drupal.org/project/drupal/issues/3354584