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: 📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed .

<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?

@smustgrave yes, all API additions or changes need a change record

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?

Removing myself from draft credit

At least the loop can be removed, so fix concerns in #10

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

+1 to the changes in #19, thanks @andypost.

I used PHPBench to generate some benchmarks for 100,000 calls. A patch for reproducing this locally is attached. Sample run:

$ vendor/bin/phpbench run core/tests/Drupal/Tests/Core/Security/DoTrustedCallbackTraitBench.php  --report=default
PHPBench (1.2.10) running benchmarks... #standwithukraine
with configuration file: /home/dave/projects/drupal8/drupal/phpbench.json
with PHP version 8.1.12, xdebug ❌, opcache ❌

\Drupal\Tests\Core\Security\DoTrustedCallbackTraitBench

    benchTrustedCallbacks # TrustedCallback.I0 - Mo0.369μs (±0.00%)
    benchTrustedCallbacks # TrustedCallback.I0 - Mo0.678μs (±0.00%)
    benchTrustedCallbacks # TrustedCallback.I0 - Mo1.266μs (±0.00%)
    benchTrustedCallbacks # TrustedCallback.I0 - Mo0.923μs (±0.00%)
    benchTrustedCallbacks # TrustedCallback.I0 - Mo1.208μs (±0.00%)
    benchTrustedCallbacks # extra_trusted_i.I0 - Mo0.318μs (±0.00%)
    benchTrustedCallbacks # extra_trusted_i.I0 - Mo0.738μs (±0.00%)
    benchTrustedCallbacks # extra_trusted_i.I0 - Mo0.494μs (±0.00%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
+------+-----------------------------+-----------------------+----------------------------------------+--------+------------+----------+--------------+----------------+
| iter | benchmark                   | subject               | set                                    | revs   | mem_peak   | time_avg | comp_z_value | comp_deviation |
+------+-----------------------------+-----------------------+----------------------------------------+--------+------------+----------+--------------+----------------+
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallbackInterface_object        | 100000 | 2,997,408b | 0.369μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallback_attribute_object       | 100000 | 2,997,424b | 0.678μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallbackInterface_static_string | 100000 | 2,997,392b | 1.266μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallbackInterface_static_array  | 100000 | 2,997,408b | 0.923μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | TrustedCallback_attribute_static_array | 100000 | 2,997,424b | 1.208μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | extra_trusted_interface_object         | 100000 | 2,997,408b | 0.318μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | extra_trusted_interface_static_string  | 100000 | 2,997,392b | 0.738μs  | +0.00σ       | +0.00%         |
| 0    | DoTrustedCallbackTraitBench | benchTrustedCallbacks | extra_trusted_interface_static_array   | 100000 | 2,997,408b | 0.494μs  | +0.00σ       | +0.00%         |
+------+-----------------------------+-----------------------+----------------------------------------+--------+------------+----------+--------------+----------------+

Checking the interface is slightly faster than using reflection on the attribute (note that in the reflection case it checks the interface first anyway), and a static string callback is almost exactly the same as the static array version with the attribute. Therefore I'm not worried about performance here at all.

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

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

Updated the MR with @andypost's changes from #19 and the additional docs requested in #27.

I disagree with adding a release note: release notes are for end users, we don't usually add release notes for developer-facing features. Although this is a nice developer feature it's not particularly notable. If we want developers to start using this in preference to the interface, then we should open followups to convert existing uses of the interface to attributes instead, and consider deprecating the interface.

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.

Release notes are also for "this might get in your way when you upgrade", and tbh I don't really think many people rely on our dev dependencies, but it's hard to know for sure. This change doesn't affect anybody directly, but I get your point that it would be nice to show off. Maybe we could tag this with "10.1 release highlights" if you think it is interesting enough, but again the highlights blog post is for end users really; maybe we need a similar blog post but aimed at developers, to highlight dev-friendly features in new minors - I really like the "A Week of Symfony" posts but we don't do anywhere near as much for developers.

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.

Tagging this for highlights, we may not do anything about it but this will remind us.

> 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 📌 Deprecate TrustedCallbackInterface in favour of TrustedCallback attribute Needs work

Automatically closed - issue fixed for 2 weeks with no activity.