Deprecate TrustedCallbackInterface in favour of 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.