Problem/Motivation

Follow-up for #3274867-35: Add TrustedCallback attribute

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

Performance concerns are resolved in #3274867-39: Add TrustedCallback attribute

Proposed resolution

- replace usage of the interface
- throw deprecation when interface is used
- add deprecation test
- deal with RenderCallbackInterface and ElementInterface in #2966711: Limit what can be called by a callback in form arrays

Remaining tasks

- patch/test/review/commit

User interface changes

no

API changes

TBD

Data model changes

no

Release notes snippet

CommentFileSizeAuthor
#30 3354584-nr-bot.txt90 bytesneeds-review-queue-bot
#10 3354584-10.patch74.67 KBandypost
#10 interdiff.txt3.92 KBandypost
#5 3354584-5.patch73.73 KBandypost

Issue fork drupal-3354584

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems
fjgarlin’s picture

fjgarlin commented

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.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Replace usage of TrustedCallbackInterface with TrustedCallback attribute » Deprecate TrustedCallbackInterface in favour of TrustedCallback attribute
Issue summary: View changes

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3354584-5.patch73.73 KB

There's extra interface and API around render elements RenderCallbackInterface

Moreover phpstan has check for the interface and will fail test

PS 47 files changed, 192 insertions, 433 deletions.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

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.
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt3.92 KB
new 3354584-10.patch74.67 KB

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

andypost opened merge request !4955

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

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

Added workaround to MR to simplify testing

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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.
andypost’s picture

andypost
he/him
Primary language Russian
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Used to split out follow-up #3392396: Improve AutowireTest to ignore TrustedCallbackInterface

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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?

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

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.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Moved condition to make check for attribute a priority

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

\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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Rebased on 11.x as fixed #3392396: Improve AutowireTest to ignore TrustedCallbackInterface

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Related issues: +#3394450: Improve the failure message from Drupal\KernelTests\Core\DependencyInjection\AutowireTest

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

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

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?

andypost’s picture

andypost
he/him
Primary language Russian
commented

@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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3354584-nr-bot.txt90 bytes

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

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.

godotislate’s picture

godotislate
he/him
commented

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Thanks, will try to revisit soon.

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

@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.

Do we strictly need this outside of ElementInterface? Maybe we change Renderer to:

return $this->doTrustedCallback($callable, $args, $message, TrustedCallbackInterface::THROW_EXCEPTION, ElementInterface::class);

and deprecate RenderCallbackInterface? But I think this is out of scope here and needs a followup to discuss.

godotislate’s picture

godotislate
he/him
commented
Issue tags: +Needs followup

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Rerun is green, moving to RTBC per #36.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Issue tags: -Needs followup

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

Updated IS, RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

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.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.