Add TrustedCallback attribute

Think this needs an issue summary with the proposed solution.

Also with the new TrustedCallback will it require a change record?

A patch from the MR. Trying to review it, but the target branch of the MR is 10.0x, and I do not have permission to edit the MR. Maybe, @longwave can do it.

I've opened an issue to decide on where to put annotation classes, so all annotation issues can work to a common standard: #3344303: [policy, no patch] Decide on a namespace for PHP Attribute classes.

<3

+++ b/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php
@@ -72,6 +74,14 @@ public function doTrustedCallback(callable $callback, array $args, $message, $er
+      if (!$safe_callback) {
+        $method = new \ReflectionMethod($object_or_classname, $method_name);
+        foreach ($method->getAttributes() as $attribute) {
+          if ($attribute->getName() === TrustedCallback::class) {
+            $safe_callback = TRUE;
+          }
+        }
+      }

Given we're doing this a runtime I wonder what the performance impact is. That said render cache exists.

For the reviewer would be helpful for the proposed solution to be added to the issue summary.

Also I previously tagged for change record is that needed?

I think a static cache can negate performance impact on repeatable callbacks

Leaning on others here for this one. But the change record has been added and makes it clear what's being added to me.

What's the future plan here?

1. TrustedCallbackInterface co-exists with the TrustedCallback attribute forever?
2. Or deprecate the TrustedCallbackInterface, and go for the TrustedCallback attribute completely?

If it's option #2, should we deprecate the TrustedCallbackInterface here?

EDIT: Just saw #2 being mentioned in the Remaining tasks of IS, sorry.

Removing myself from draft credit

(EDIT again. Sorry, it seems I can not do it. The committer does it, please.)

At least the loop can be removed, so fix concerns in #10, ref https://www.php.net/manual/en/reflectionfunctionabstract.getattributes.php

I think we can deprecate interface but still needs to profile somehow reflection vs interface check + method call

Interesting that callback as array vs string is ~50% faster

TrustedCallbackInterface_static_string 1.266μs
TrustedCallbackInterface_static_array 0.923μs
...
extra_trusted_interface_static_string 0.738μs
extra_trusted_interface_static_array 0.494μs

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I find it ready but as my patch is the last, I can't RTBC

Patch 19 LGTM.

We need some documentation updates.

• The class docs on \Drupal\Core\Security\DoTrustedCallbackTrait need updating
• The method docs on \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback need updating
• We should add a release note so it's easy to list this in the changes for 10.1.x

I ummed and ahhed on the release note - fwiw if "release notes are for end users, we don't usually add release notes for developer-facing features" is true then we should not do release notes for dev dependency updates either - the fact that we do is why I decided to add the tag as this is the first use of an attribute in core. Happy to remove the tag as I'm not a release manager.

Looks docs are improved

Committed 2420d38 and pushed to 10.1.x. Thanks!

I'm happy to leave this small thing out of release notes etc.. blocks and routes as attributes are more important and things I think we should highlight.

Nice to see us adopting new language features! :D

I think #28 through #32 are probably why the change record is not yet published?

@Wim Leers nah I just forgot to publish it :)

We could file a follow-up to convert core's usages of TrustedCallbackInterface and deprecate it.

As #21 shows - no reason to replace all as reflection x3 slower then interface but some places could use it

Filed follow-up #3354584: Deprecate TrustedCallbackInterface in favour of TrustedCallback attribute

> As #21 shows - no reason to replace all as reflection x3 slower then interface but some places could use it

This should maybe have been documented on the attribute class so developers know not to use it in performance-critical pathways.

@joachim I think the performance concerns are overblown. It's looks like it might be x2 as slow but we're dealing less than 1 microsecond here and I won't be surprised if there was variation. When I run this locally I get;

+------+-----------------------------+-----------------------+----------------------------------------+---------+------------+----------+--------------+----------------+
| iter | benchmark                   | subject               | set                                    | revs    | mem_peak   | time_avg | comp_z_value | comp_deviation |
+------+-----------------------------+-----------------------+----------------------------------------+---------+------------+----------+--------------+----------------+
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallbackInterface_object        | 1000000 | 1,714,520b | 0.348μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallback_attribute_object       | 1000000 | 1,714,536b | 0.543μs  | +0.00σ       | +0.00%         |

| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallbackInterface_static_array  | 1000000 | 1,714,520b | 0.766μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallback_attribute_static_array | 1000000 | 1,714,536b | 0.992μs  | +0.00σ       | +0.00%         |
+------+-----------------------------+-----------------------+----------------------------------------+---------+------------+----------+--------------+----------------+

(reducing the table to compare things that should be compared as maybe @andypost has compared apples with oranges). I think from this we can say that using an attribute will cost about 0.2μs per attribute. Given these are used for render callbacks this is likely to be an incredibly small cost even if you have 100 methods using this attribute and all 100 are being used to render a page. And even then non of these will be called if the render is cached in some way (page cache, dynamic page cache).

Additionally the attribute test is not correct because the object that is being used also has the TrustedCallbackInterface so it has to do more work that it should. If you are using attributes you should not be using the interface. Plus if we didn't use the interface then we wouldn't need to do the is_subclass_of() so that'd save more time. If we change the logic around in \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback() to prioritise attribute checking over TrustedCallbackInterface checking and we also test an object only using attributes then we can see something like:

+------+-----------------------------+-----------------------+----------------------------------------+---------+------------+----------+--------------+----------------+
| iter | benchmark                   | subject               | set                                    | revs    | mem_peak   | time_avg | comp_z_value | comp_deviation |
+------+-----------------------------+-----------------------+----------------------------------------+---------+------------+----------+--------------+----------------+
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallbackInterface_object        | 1000000 | 1,715,968b | 0.453μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallback_attribute_object       | 1000000 | 1,715,984b | 0.479μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallback_only_attribute_object  | 1000000 | 1,716,016b | 0.476μs  | +0.00σ       | +0.00%         |
+------+-----------------------------+-----------------------+----------------------------------------+---------+------------+----------+--------------+----------------+

So yeah I think we should deprecate the interface in preference for the attribute.

FWIW here's the code for I had in the relevant part of \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback()

    if (isset($method_name)) {
      if ($extra_trusted_interface && is_subclass_of($object_or_classname, $extra_trusted_interface)) {
        $safe_callback = TRUE;
      }

      if (!$safe_callback) {
        $method = new \ReflectionMethod($object_or_classname, $method_name);
        $safe_callback = (bool) $method->getAttributes(TrustedCallback::class);
      }

      if (!$safe_callback && is_subclass_of($object_or_classname, TrustedCallbackInterface::class)) {
        if (is_object($object_or_classname)) {
          $methods = $object_or_classname->trustedCallbacks();
        }
        else {
          $methods = call_user_func($object_or_classname . '::trustedCallbacks');
        }
        $safe_callback = in_array($method_name, $methods, TRUE);
      }
    }
    elseif ($callback instanceof \Closure) {
      $safe_callback = TRUE;
    }

Thank you for good news, then we can deprecate interface in #3354584: Deprecate TrustedCallbackInterface in favour of TrustedCallback attribute