Replace usage of TrustedCallbackInterface with TrustedCallback attribute

Note that we might run into this issue when running the tests: https://github.com/mglaman/phpstan-drupal/issues/527
I ran into it when trying to use the new method.

The new code does work in Drupal, but it wouldn't pass CI validation.

Once it's fixed then everything is good but I thought I'd mention it just in case.

Better title and updated summary

As measurements in #3274867-39: Add TrustedCallback attribute → and comment #40 showing we can deprecate interface as well

Filed CR https://www.drupal.org/node/3355686 → but probably we can improve existing one https://www.drupal.org/node/3349470 →

There's extra interface and API around render elements RenderCallbackInterface

Moreover phpstan has check for the interface and will fail test

phpstan needs improvement first

  Line   core/lib/Drupal/Core/Access/RouteProcessorCsrf.php                    
 ------ ---------------------------------------------------------------------- 
  52     #lazy_builder callback class 'Drupal\Core\Access\RouteProcessorCsrf'  
         at key '0' does not implement                                         
         Drupal\Core\Security\TrustedCallbackInterface.                        
         💡 Change record: https://www.drupal.org/node/2966725.  

phpstan-drupal issue: https://github.com/mglaman/phpstan-drupal/issues/527

I'm working on support in this PR https://github.com/mglaman/phpstan-drupal/pull/529

PHPStan doesn't have an API for reading attributes from its reflection API yet. Or at least I cannot find it. So it's going to require a bit of work and refactor of the existing rule. This is needed anyway to handle date_time_callbacks and date_date_callbacks requiring this anyway.

The phpstan work referenced in #7 has been merged and closed.

Locally it still fails

Running PHPStan on changed files.
 ------ --------------------------------------------------------------------------------------------------------------------------------- 
  Line   lib/Drupal/Core/Security/DoTrustedCallbackTrait.php (in context of class Drupal\Tests\Core\Security\DoTrustedCallbackTraitTest)  
 ------ --------------------------------------------------------------------------------------------------------------------------------- 
  75     Call to deprecated method trustedCallbacks() of class Drupal\Core\Security\TrustedCallbackInterface:                             
         in drupal:10.1.0 and is removed from drupal:11.0.0. Instead,                                                                     
           you should use \Drupal\Core\Security\Attribute\TrustedCallback attribute                                                       
           for the method.                                                                                                                
 ------ --------------------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------------------- 
  Line   tests/Drupal/Tests/Core/Render/RendererTest.php                                                                                      
 ------ ------------------------------------------------------------------------------------------------------------------------------------- 
  623    #access_callback callback class 'Drupal\\Tests\\Core…' at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.  
         💡 Change record: https://www.drupal.org/node/2966725.                                                                               
  623    #access_callback callback class 'Drupal\\Tests\\Core…' at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.  
         💡 Change record: https://www.drupal.org/node/2966725.                                                                               
  623    #access_callback callback class 'Drupal\\Tests\\Core…' at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.  
         💡 Change record: https://www.drupal.org/node/2966725.                                                                               
  623    #access_callback callback class 'Drupal\\Tests\\Core…' at key '0' does not implement Drupal\Core\Security\TrustedCallbackInterface.  
         💡 Change record: https://www.drupal.org/node/2966725.                                                                               
 ------ ------------------------------------------------------------------------------------------------------------------------------------- 

@mglaman still reported https://git.drupalcode.org/issue/drupal-3354584/-/pipelines/27567/codequ...

Added workaround to MR to simplify testing

One test still fails \Drupal\KernelTests\Core\DependencyInjection\AutowireTest looks like it needs fixes as this services has no interface anymore

The failed test looks like has wrong expectations, specifically looking at

'Drupal\Core\Security\TrustedCallbackInterface' =>
    'announcements_feed.lazy_builders'

the whole test from https://git.drupalcode.org/issue/drupal-3354584/-/jobs/149954

Fail      Other      phpunit-328.xml      0 Drupal\KernelTests\Core\DependencyI
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.8 by Sebastian Bergmann
    and contributors.
    
    Testing Drupal\KernelTests\Core\DependencyInjection\AutowireTest
    .F                                                                  2 / 2
    (100%)
    
    Time: 00:01.762, Memory: 4.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\KernelTests\Core\DependencyInjection\AutowireTest::testCoreServiceAliases
    Failed asserting that two arrays are identical.
    --- Expected
    +++ Actual
    @@ @@
         'Drupal\user\UserDataInterface' => 'user.data'
         'Drupal\user\UserAuthInterface' => 'user.auth'
         'Drupal\user\PermissionHandlerInterface' => 'user.permissions'
    -    'Drupal\user\ToolbarLinkBuilder' => 'user.toolbar_link_builder'
         'Drupal\user\UserFloodControlInterface' => 'user.flood_control'
         'Drupal\user\ModulePermissionsLinkHelper' =>
    'user.module_permissions_link_helper'
         'Drupal\update\UpdateManagerInterface' => 'update.manager'
    @@ @@
         'Drupal\system\SecurityAdvisories\SecurityAdvisoriesFetcher' =>
    'system.sa_fetcher'
         'Drupal\system\ModuleAdminLinksHelper' =>
    'system.module_admin_links_helper'
         'Drupal\statistics\StatisticsStorageInterface' =>
    'statistics.storage.node'
    -    'Drupal\shortcut\ShortcutLazyBuilders' => 'shortcut.lazy_builders'
         'Drupal\serialization\EntityResolver\ChainEntityResolverInterface' =>
    'serializer.entity_resolver'
         'Drupal\search\SearchPageRepositoryInterface' =>
    'search.search_page_repository'
         'Drupal\search\SearchIndexInterface' => 'search.index'
    @@ @@
         'Drupal\file\FileRepositoryInterface' => 'file.repository'
         'Drupal\file\Validation\RecursiveValidatorFactory' =>
    'file.recursive_validator_factory'
         'Drupal\file\Validation\FileValidatorInterface' => 'file.validator'
    -    'Drupal\editor\Element' => 'element.editor'
         'Drupal\content_translation\FieldTranslationSynchronizerInterface' =>
    'content_translation.synchronizer'
         'Drupal\content_translation\ContentTranslationManagerInterface' =>
    'content_translation.manager'
         'Drupal\content_translation\BundleTranslationSettingsInterface' =>
    'content_translation.manager'
    @@ @@
         'Drupal\config_translation\ConfigMapperManagerInterface' =>
    'plugin.manager.config_translation.mapper'
         'Drupal\comment\CommentManagerInterface' => 'comment.manager'
         'Drupal\comment\CommentStatisticsInterface' => 'comment.statistics'
    -    'Drupal\comment\CommentLazyBuilders' => 'comment.lazy_builders'
         'Drupal\comment\CommentLinkBuilderInterface' => 'comment.link_builder'
         'Drupal\ckeditor5\Plugin\CKEditor5PluginManagerInterface' =>
    'plugin.manager.ckeditor5.plugin'
         'Drupal\ckeditor5\SmartDefaultSettings' =>
    'ckeditor5.smart_default_settings'
    @@ @@
         'Drupal\big_pipe\Render\BigPipe' => 'big_pipe'
         'Drupal\ban\BanIpManagerInterface' => 'ban.ip_manager'
         'Drupal\announcements_feed\AnnounceFetcher' =>
    'announcements_feed.fetcher'
    -    'Drupal\Core\Security\TrustedCallbackInterface' =>
    'announcements_feed.lazy_builders'
         'Drupal\Core\Cache\VariationCacheFactoryInterface' =>
    'variation_cache_factory'
         'Drupal\Core\Cache\Context\CacheContextsManager' =>
    'cache_contexts_manager'
         'Drupal\Core\Cache\CacheTagsChecksumInterface' =>
    'cache_tags.invalidator.checksum'
    
    /builds/issue/drupal-3354584/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/builds/issue/drupal-3354584/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/issue/drupal-3354584/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutowireTest.php:113
/builds/issue/drupal-3354584/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 2, Assertions: 5, Failures: 1.

This services need aliases so added, maybe it makes sense to add condition for TrustedCallbackInterface to expectation, so services implementing this interface also will require aliasing

Used to split out follow-up 🐛 Improve AutowireTest to ignore TrustedCallbackInterface Needs review

One thing I'm wondering here is how this works with inheritance. The method also worked if you have a subclass and override the callback there. I just verified that this doesn't work with attributes: https://3v4l.org/42bu0

Apparently this isn't something that happens with core, but especially views uses this a lot on all many plugin base classes, so there's definitely a non-zero chance that contrib/custom code has changed them? Would we need a BC layer that explicitly checks parent classes implementation?

Thanks, good point, maybe we should keep interface usage but check for attribute before BC

Created https://github.com/mglaman/phpstan-drupal/issues/611 to request a phpstan rule that checks the parent method. If we are going to deprecate the interface we need something.

I don't have much hands on experience with the PHP Reflection library - is is possible to recurse up the inheritance chain?

Or could we simply allow overrides to fail if the extending class fails to add the attribute just like we have failures if either the trustedCallbacks method either fails to be implemented for new callbacks, or is implemented but fails to call the parent method to initiate the list.

The problem is that the current patch would immediately fail on a minor release, it wouldn't be a deprecation.

But yes, if we either keep the interface and method for now but check the attribute first, which we should anyway now for performance reasons or check the hierarchy at least for BC then that would work.

Thanks for clarifying @Berdir - yes we should deprecate but retain the interface and remove it in 11.x

Moved condition to make check for attribute a priority

\Drupal\Core\Render\Renderer::doCallback() and \Drupal\Core\Render\Element\RenderCallbackInterface needs work to remove mentions

maybe makes sense to introduce RenderCallback attribute at the same time

Rebased on 11.x as fixed 🐛 Improve AutowireTest to ignore TrustedCallbackInterface Needs review

Needs review for #24 and related context 📌 Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems Fixed

maybe makes sense to introduce RenderCallback attribute at the same time

Could we not also refactor uses of RenderCallback to use #[TrustedCallback]? Is there some separation of concerns here that warrants two attributes?

@FatherShawn interesting, sadly they are coupled but maybe we need more cuts for that

split it
- priority of attribute before interface check (possible for 10.2)
- separation of RenderCallbackInterface from TrustedCallbackInterface
- the deprecation

Re #17 do we need additional test coverage for the inherited case?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Addressed #17 by explicitly checking parent methods for attributes, which works for now but we issue a deprecation if the parent has the attribute but the child method does not. Added test coverage for this, also bumped the other deprecation to 12.x.

Nits/questions about which CR https://www.drupal.org/node/3355686 → or https://www.drupal.org/node/3349470 → should be used in deprecation messages. Looks good otherwise.

Thanks, will try to revisit soon.

The only remaining question is about RenderCallbackInterface which would be great to move as well.
Or at least follow-up is required

@godotislate Thanks, updated all references to point to the deprecation. Also updated the deprecation CR to explicitly mention versions and point back to the original CR as an example for conversion.

@andypost Not sure what to do about that, maybe wait for followup? We could allow #[TrustedCallback] on the class to allow all methods in the class - but then we run into the inheritance problem that @berdir describes immediately, because:

interface ElementInterface extends PluginInspectionInterface, RenderCallbackInterface {

We don't want all element implementations to have to specify #[TrustedCallback] on the class.

Changes in MR and CR look good to me. I agree that what to do with RenderCallbackInterface or ElementInterface should be handled in a follow up.

Test failure looks to be unrelated, so I think it just needs to be re-run. Once it passes, I think it's good to be moved to RTBC.

Rerun is green, moving to RTBC per #36.

The follow-up exists 📌 Limit what can be called by a callback in form arrays Needs work and there's some work already done

Updated IS, RTBC++

I'm not sure that this deprecation is being done correctly. I think we need to leave the interface and static methods in place. I think we should issue a deprecation when using the interface and no attribute is present on the method (this will make the inheritance case simpler). And then deprecate the interface in 12 and remove in 13. That way we're not breaking API (as removal of an interface is). Consider the case of a class in contrib that extends from a core class and overrides \Drupal\Core\Security\TrustedCallbackInterface::trustedCallbacks() to add new trusted callbacks. That would be broken by the proposed change here.