Fix access checks for bundle permissions to avoid triggering a config validation error

Created on 27 August 2022, over 2 years ago
Updated 4 August 2024, 5 months ago

Problem/Motivation

Checking access to /admin/structure/comment/manage/comment/display logs the warning message

Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies.

Using Drupal 10.3 and the contrib admin_toolbar module, the access check and the warning message are triggered on every page load for an admin user.

Here is a partial stack trace for the warning message:

Drupal\Core\Entity\EntityDisplayBase->onDependencyRemoval()
Drupal\Core\Config\ConfigManager->callOnDependencyRemoval()
Drupal\Core\Config\ConfigManager->getConfigEntitiesToChangeOnDependencyRemoval()
Drupal\user\Form\EntityPermissionsForm->permissionsByProvider()
Drupal\user\Form\EntityPermissionsForm->access()

Steps to reproduce

These steps trigger the warning once, since Drupal 9.4.0:

  1. Install Drupal with the standard profile.
  2. Browse to Administration > Structure > Comment types > Default comments > Manage display: /admin/structure/comment/manage/comment/display
  3. Check error logs: /admin/reports/dblog

These steps trigger the warning on every page load:

  1. Install Drupal 10.3 with the standard profile.
  2. Add the Admin Toolbar module.
  3. Enable the admin_toolbar and admin_toolbar_tools modules.
  4. Log in as an admin user.
  5. Visit /admin/reports/dblog and reload the page a few times.

This only happens on Drupal 10.3 (and 11.x) thanks to 🐛 [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed .

Proposed resolution

In Drupal\user\Form\EntityPermissionsForm::permissionsByProvider(), call Drupal\Core\Config\ConfigManager::findConfigEntityDependencies() or Drupal\Core\Config\ConfigManager::findConfigEntityDependenciesAsEntities() instead of Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval().

Remaining tasks

We might want to add follow-up issues for the following:

  1. Update Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval() so that it does not log warning messages, or find a way to suppress those messages. See Comment #66.
  2. Update Drupal\comment\Tests\CommentTestTrait::addDefaultCommentField() to use the 'default' view mode by default, not 'full'. See Comment #61.

User interface changes

None

API changes

None, unless (2) from the Remaining tasks is done as part of this issue.

Data model changes

None

Release notes snippet

N/A

Original report by DYdave

Initial investigation

After doing an initial investigation, the error message would seem to come from the following piece of code:
https://git.drupalcode.org/project/drupal/-/blob/9.4.5/core/lib/Drupal/C...

          // If there are unresolved deleted dependencies left, disable this
          // component to avoid the removal of the entire display entity.
          if ($this->getPluginRemovedDependencies($renderer->calculateDependencies(), $dependencies)) {
            $this->removeComponent($name);
            $arguments = [
              '@display' => (string) $this->getEntityType()->getLabel(),
              '@id' => $this->id(),
              '@name' => $name,
            ];
            $this->getLogger()->warning("@display '@id': Component '@name' was disabled because its settings depend on removed dependencies.", $arguments);
            $changed = TRUE;
          }

It would seem a lot of this code relates to issue: #2562107-77: EntityDisplayBase should react on removal of its components dependencies .

After doing some basic debugging, breaking at this point in code resulted in the following stack trace:

array:55 [▼
0 => array:7 [▼
"file" => "/web/core/lib/Drupal/Core/Config/ConfigManager.php"
"line" => 497
"function" => "onDependencyRemoval"
"class" => "Drupal\Core\Entity\EntityDisplayBase"
"object" => Drupal\Core\Entity\Entity\EntityViewDisplay {#2650 ▶}
    "type" => "->"
    "args" => array:1 [▶]
  ]
  1 => array:7 [▼
    "file" => "/web/core/lib/Drupal/Core/Config/ConfigManager.php"
    "line" => 360
    "function" => "callOnDependencyRemoval"
    "class" => "Drupal\Core\Config\ConfigManager"
    "object" => Drupal\Core\Config\ConfigManager {#857 ▶}
    "type" => "->"
    "args" => array:4 [▶]
  ]
  2 => array:7 [▼
    "file" => "/web/core/modules/user/src/Form/EntityPermissionsForm.php"
    "line" => 88
    "function" => "getConfigEntitiesToChangeOnDependencyRemoval"
    "class" => "Drupal\Core\Config\ConfigManager"
    "object" => Drupal\Core\Config\ConfigManager {#857 ▶}
    "type" => "->"
    "args" => array:2 [▶]
  ]
  3 => array:7 [▼
    "file" => "/web/core/modules/user/src/Form/EntityPermissionsForm.php"
    "line" => 173
    "function" => "permissionsByProvider"
    "class" => "Drupal\user\Form\EntityPermissionsForm"
    "object" => Drupal\user\Form\EntityPermissionsForm {#2057 ▶}
    "type" => "->"
    "args" => []
  ]
  4 => array:5 [▼
    "function" => "access"
    "class" => "Drupal\user\Form\EntityPermissionsForm"
    "object" => Drupal\user\Form\EntityPermissionsForm {#2057 ▶}
    "type" => "->"
    "args" => array:3 [▶]

    ...

 
Mostly, with this chain of calls:

[... Multiple calls before ...]
Drupal\user\Form\EntityPermissionsForm:permissionsByProvider
Drupal\Core\Config\ConfigManager:getConfigEntitiesToChangeOnDependencyRemoval
Drupal\Core\Config\ConfigManager:callOnDependencyRemoval
Drupal\Core\Entity\EntityDisplayBase:onDependencyRemoval

 
The code of these functions seems to be related to #2850973: ConfigEntityInterface::onDependencyRemoval() called with incorrect dependency list .
 
In short, the reason the problem happens is because:
When the Manage Display Comment page is displayed, a list of all config depending on 'comment.type.comment' is created.
Since the default display of comments 'core.entity_view_display.comment.comment.default' depends on 'comment.type.comment' and since 'core.entity_view_display.node.article.default' depends on 'core.entity_view_display.comment.comment.default' it is also added to the list of dependencies.
core.entity_view_display.node.article.default > core.entity_view_display.comment.comment.default > comment.type.comment
 
The affected "cross" dependency is due to the fact the Node, type Article, default display 'core.entity_view_display.node.article.default' uses the default comment formatter (to display the Comment field), which points to the default display of the Comment, type Comment 'core.entity_view_display.comment.comment.default', see:
https://git.drupalcode.org/project/drupal/-/blob/9.4.5/core/modules/comm...
In particular lines around 270:

        if ($display = EntityViewDisplay::load("comment.$bundle.$mode")) {
          $dependencies[$display->getConfigDependencyKey()][] = $display->getConfigDependencyName();
        }

 
From a logic perspective it certainly makes sense to allow the default comment formatter, and Plugins in general to react to configuration changes on Entity Displays.
For example, it would probably make sense adding a specific handling case for the default comment formatter: Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter:onDependencyRemoval, to check whether the referenced display still exists and capture any changes to the settings, much like it's currently done for the 'ImageFormatter', see:
https://git.drupalcode.org/project/drupal/-/blob/9.4.5/core/modules/imag...
 

Work-around/Temporary solution

Note the problem can be fixed temporarily by removing the config dependency either programmatically, or by importing the modified config through the backend (at '/admin/config/development/configuration/single/import/') or from its file (edit file 'config/sync/core.entity_view_display.node.article.default.yml', remove the line 'core.entity_view_display.comment.comment.default' under 'config' and import).
But of course the config is added back the next time the Node Article display 'core.entity_view_display.node.article.default' is saved.
 
Lastly, since we're really unsure how to approach the problem and the config dependency system seems to have a lot of moving parts, we found the following issues that could potentially be related or where some work is carried around Plugins 'getPluginRemovedDependencies' and 'getPluginRemovedDependencies':

 

We would greatly appreciate to have your feedback and more particularly, if anyone would have any pointers or guidance on what could be the best approach to finding a solution to the problem.
Feel free to let us know if you have any questions, if anything is unclear or if more information would be needed, we would surely be glad to help.
Thanks in advance.

🐛 Bug report
Status

Fixed

Version

10.3

Component
User system 

Last updated 5 days ago

Created by

🇫🇷France dydave

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States SocialNicheGuru

    How can you find out what dependencies are missing?

  • 🇺🇸United States SocialNicheGuru

    [WORK AROUND]
    I ended up creating my own content type and replacing the core comment content type in my fields. I no longer see the errors

  • 🇮🇷Iran tsotoodeh

    Hey guys
    Same issue observed in 9.5.3 and 10.0.3 versions. Any workaround yet?

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    This is a warning coming from access checking of comment-types for the 'permissions' tab.

    If you don't really want permissions tab for comment-types, you can add this to a custom module.

    
    function my_module_entity_type_alter(array &$entity_types): void {
      if (isset($entity_types['comment_type'])) {
        assert($entity_types['comment_type'] instanceof \Drupal\Core\Config\Entity\ConfigEntityTypeInterface);
        $route_providers = $entity_types['comment_type']->getRouteProviderClasses();
        unset($route_providers['permissions']);
        $entity_types['comment_type']->setHandlerClass('route_provider', $route_providers);
      }
    }
    
    

    You will lose the 'Permissions' tab for comment types, but its probably not an issue.

  • 🇦🇺Australia fenstrat Australia

    In theory 📌 Return early in EntityPermissionsForm::access if the user does not have "administer permissions" Fixed could fix this. Also being discussed there is the potential for removing the whole dynamic hiding of the permission local task all together (which is what triggers this issue).

  • 🇨🇭Switzerland berdir Switzerland

    I was going to comment that removing the call there should still result in the same problem in other (although rarer) cases, but only in expected scenarios.

    What's happening is that the permission access stuff is asking for config that needs to be updated or deleted if the comment type were to be deleted. That includes its view and form displays, then removing those will result in following that trail to the node type display and it's rendering of comments, which depends on that comment view display, it can't reconfigure it self to something else, so it gets disabled. The UI doesn't even allow you to delete a comment type while there are still fields using it, so you can't generate the same error there, not in the UI anyway. And while there are other situations where this could happen with other formatters, as long as it's called only on the delete confirm form, it's a much, muss less severe problem, the only possible thing to improve would be how this is presented.

  • 🇬🇧United Kingdom very_random_man

    Just here to say that I was getting this issue on Drupal 10.2.4 and the workaround in #14 still does the job. Thanks!

  • 🇬🇧United Kingdom jaydenpearly

    Drupal 10.2.7 is ok but this problem is still observed in version 10.3.
    #15 doesn't work for 10.3. It could not apply patch.

  • 🇺🇸United States amanire

    Noting that the workaround in #14 also resolved this for me. But that it took me a while to figure out how to reproduce and confirm because I was not creating watchdog entries with the warning when logged in with the "Administrator" role. Once I realized that this was happening, I tried logging in with another user account with fewer permissions and successfully reproduced the warnings in the description.

  • 🇮🇹Italy kopeboy Milan

    This is a very annoying error considering it happens on a fresh install of Drupal core only (10.3.0-rc1), ie. on ANY website, for the site builder.

    After adding a few modules you can even easily get out of memory errors (>1GB) for doing a Flush all caches from the Admin toolbar link (with the site still being blank!).

  • 🇸🇰Slovakia kaszarobert

    Just updated to 10.3.0 and suddenly this error fills the logs.

  • 🇬🇷Greece arx-e

    Also updated to 10.3.0 today and I have white screens on the main theme and lots of entries of this error.

  • 🇩🇪Germany butch.coolidge Reutlingen

    Same here on several installations after upgrading to 10.3,

  • 🇺🇸United States aitala

    Another instance of this message popping up on a 10.3 update...

    Whee!

    Eric

  • 🇷🇴Romania idflorin

    Drupal 10.2.7 is okay, but this problem reappeared after updating to 10.3.
    Entity view display 'node.story.default': Component 'comment' was disabled because its settings depend on removed dependencies.

  • 🇺🇸United States bobburns

    Same issue after upgrade to 10.3 of as 23, 24, 25, 26, 27

  • 🇬🇧United Kingdom catch

    Given this bug is two years old but lots of people are just hitting it now, it feels like it's being triggered by something. The mention of router access makes me wonder if it's 🐛 Admin page access denied even when access is given to child items RTBC - could someone try reverting the commit from that issue to see if it helps?

    Bumping this to critical - even if the original bug maybe isn't, the side effects combined with whatever is triggering it more in 10.3 are.

  • 🇺🇸United States bobburns

    That patch is committed to core and is already applied to 10.3 - I checked the code

    I built a mini custom module with the function from #14 and called it "comment_block" with just an info file an module with the function and it has removed the errors - but of course does not answer what is causing it

    comment_block (in modules directory)
    comment_block.info.yml

    name: 'Comment Block'
    description: 'Blocks comment dependency error'

    core_version_requirement: ">=8"
    type: module

    comment_block.module

    <?php

    // https://www.drupal.org/project/drupal/issues/3306434 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed

    function comment_block_entity_type_alter(array &$entity_types): void {
    if (isset($entity_types['comment_type'])) {
    assert($entity_types['comment_type'] instanceof \Drupal\Core\Config\Entity\ConfigEntityTypeInterface);
    $route_providers = $entity_types['comment_type']->getRouteProviderClasses();
    unset($route_providers['permissions']);
    $entity_types['comment_type']->setHandlerClass('route_provider', $route_providers);
    }
    }

    install it and the errors are gone

  • 🇬🇧United Kingdom catch

    That patch is committed to core and is already applied to 10.3 - I checked the code

    Yes I know, I'm wondering if it caused the problem here, and asked if someone could test reverting it.

  • 🇳🇿New Zealand quietone

    I tested by reverting the commit for 🐛 Admin page access denied even when access is given to child items RTBC , doing a standard install and following the steps in the issue summary. The failure persists.

    Bit did you see comment #4? That used git bisect, to track this to 91fdb8a #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions .

  • 🇨🇭Switzerland berdir Switzerland

    I think there are two different issues here.

    a) That we attempt to change config like that at runtime just to check if there are permissions. This is using the config API in a way that's not intended and really bad for performance. That's cased by the manage permissions issue, and 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work would "fix" that I believe. It stalled out unfortunately.

    b) The fact that this view display does have invalid config that causes it to trigger an automated change. That's something that we should/could fix here.

    I didn't look into what the problem exactly is, but it could also be triggered in another scenario so it makes sense to make both changes, a) for performance and b) for consistency.

  • 🇳🇿New Zealand quietone

    Ah, thanks. I applied the diff from 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work locally and the test here now passes. Maybe close this in favor of that? It is too late here to think about that.

  • 🇬🇧United Kingdom kmv

    NB: the workaround in #12 does not work for me, the warning moves to the new comment.

  • 🇳🇿New Zealand davidwhthomas

    Just noting I also hit this issue after the Drupal core 10.3.0 upgrade today. It caused significant noticeable site slowdown with many similar log entries.
    The workaround approach in #14 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed helped to fix the issue for now, with thanks.

  • 🇺🇸United States adrianm6254

    I tried #14 and that did not resolve my issue that only started when I installed 10.3.0-rc1.

    I am getting this same error on every site I have upgraded.

  • 🇦🇹Austria agoradesign

    happened to me in 10.3.0 too, #14 didn't help though.

    I had a look at the stack trace, it is related to comment_type's route access checking. admin_toolbar is calling the menu link tree's transform():

        #0 /app/web/core/lib/Drupal/Core/Config/ConfigManager.php(489): Drupal\Core\Entity\EntityDisplayBase->onDependencyRemoval(Array)
    #1 /app/web/core/lib/Drupal/Core/Config/ConfigManager.php(352): Drupal\Core\Config\ConfigManager->callOnDependencyRemoval(Object(Drupal\Core\Entity\Entity\EntityViewDisplay), Array, 'config', Array)
    #2 /app/web/core/modules/user/src/Form/EntityPermissionsForm.php(96): Drupal\Core\Config\ConfigManager->getConfigEntitiesToChangeOnDependencyRemoval('config', Array)
    #3 /app/web/core/modules/user/src/Form/EntityPermissionsForm.php(185): Drupal\user\Form\EntityPermissionsForm->permissionsByProvider()
    #4 [internal function]: Drupal\user\Form\EntityPermissionsForm->access(Object(Symfony\Component\Routing\Route), Object(Drupal\Core\Routing\RouteMatch), NULL)
    #5 /app/web/core/lib/Drupal/Core/Access/CustomAccessCheck.php(84): call_user_func_array(Array, Array)
    #6 [internal function]: Drupal\Core\Access\CustomAccessCheck->access(Object(Symfony\Component\Routing\Route), Object(Drupal\Core\Routing\RouteMatch), Object(Drupal\Core\Session\AccountProxy), NULL)
    #7 /app/web/core/lib/Drupal/Core/Access/AccessManager.php(160): call_user_func_array(Array, Array)
    #8 /app/web/core/lib/Drupal/Core/Access/AccessManager.php(136): Drupal\Core\Access\AccessManager->performCheck('access_check.cu...', Object(Drupal\Component\Utility\ArgumentsResolver))
    #9 /app/web/core/lib/Drupal/Core/Access/AccessManager.php(93): Drupal\Core\Access\AccessManager->check(Object(Drupal\Core\Routing\RouteMatch), Object(Drupal\Core\Session\AccountProxy), NULL, true)
    #10 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(218): Drupal\Core\Access\AccessManager->checkNamedRoute('entity.comment_...', Array, Object(Drupal\Core\Session\AccountProxy), true)
    #11 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(107): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->menuLinkCheckAccess(Object(Drupal\Core\Menu\MenuLinkDefault))
    #12 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(111): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
    #13 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(111): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
    #14 /app/web/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php(111): Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
    #15 [internal function]: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators->checkAccess(Array)
    #16 /app/web/core/lib/Drupal/Core/Menu/MenuLinkTree.php(153): call_user_func(Array, Array)
    #17 /app/web/modules/contrib/admin_toolbar/src/Render/Element/AdminToolbar.php(47): Drupal\Core\Menu\MenuLinkTree->transform(Array, Array)
    #18 [internal function]: Drupal\admin_toolbar\Render\Element\AdminToolbar::preRenderTray(Array)

    🐛 Admin Toolbar Tools breaks Dynamic Page Cache on Drupal 10.3 when update module is enabled Active is definitely related imho. A comment there is pointing to 🐛 [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed as the root - looking at the changes made there, compared with the calls in the stack trace, this seems very realistic to me

  • 🇺🇸United States CProfessionals

    same issue here 10.2.5 > 10.3.0

  • 🇨🇭Switzerland berdir Switzerland

    The correct issue that fixes this is 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work , admin_toolbar isn't the problem, it just exposes this.

    Ah, thanks. I applied the diff from #3384600: Don't hide permissions local tasks on bundles when no permissions are defined locally and the test here now passes. Maybe close this in favor of that? It is too late here to think about that.

    I orignally thought that there's an issue on how the config for comment formatter is defined, but no, everything is fine here. That referenced issue is the only thing we need to fix. It specifically asks the config management system what it needs to do if the comment.type.comment config gets deleted, which bubbles up and results in removing the config for the comment formatter.

    As mentioned over there, this API is just not meant to be used like this, it's slow and actively does way too much stuff way too much for this to be used as an access check.

  • 🇺🇸United States sjhuskey

    Forgive me for being dim, but the issue cited by #41 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed has to do with Drupal core 11.x-dev, doesn't it? Those who have posted earlier in this thread have been concerned about 10.3 and previous versions.

    I implemented the custom module solution as suggested by #30 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed on my 10.3 site. It seems to have resolved the issue.

  • 🇺🇸United States mortona2k Seattle

    I was able to apply the patch from #3384600 to 10.3, but now I see a deprecation message instead of the warning:

    Debug: Drupal\user\Form\EntityPermissionsForm:access() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use a permissions check on the route definition instead. See https://www.drupal.org/node/3384745 in Drupal\user\Form\EntityPermissionsForm->access() (line 168 of core/modules/user/src/Form/EntityPermissionsForm.php).

  • 🇺🇸United States FrankieD3

    Applying the patch from #3384600 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work on my 10.3.0 install has resolved the issue for me as well.

  • 🇩🇪Germany Anybody Porta Westfalica

    I can also confirm that the patch from 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work solved the issue for us. We had this for forum nodes.

  • 🇸🇮Slovenia KlemenDEV

    Happening since updating to 10.3 from 10.2. Site is super slow due to all of logging for all comment types and display modes of nodes that contain those comment types for every node visit

  • 🇸🇮Slovenia KlemenDEV

    Patch #5 from https://www.drupal.org/project/drupal/issues/3384600 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work fixes the problem described in https://www.drupal.org/project/drupal/issues/3306434 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed that started happening when updating from 10.2 to 10.3

  • 🇺🇸United States bobburns

    I installed the 338400 patch and then uninstalled the custom module I made and no errors have returned

    It threw user notices into the log one time

    Since this is a core patch, I keep a directory in the modules directory with patches because on the next core upgrade if this is not committed to core the issue will return, and I do not want to dig like a pig for tyruffels to find the patch again

  • I just wanted to mention that your step by step comment was so easy a dead jellyfish could do it, so I didn't have any problems.

    Thank you for taking the time.

    https://www.drupal.org/project/drupal/issues/3306434#comment-15651457 🐛 Fix access checks for bundle permissions to avoid triggering a config validation error Fixed

  • 🇸🇮Slovenia KlemenDEV

    I hope #3384600 lands soon as this prevents 10.3 from being "usable" due to the amount of logging that happens with this bug

  • 🇮🇳India newswatch Delhi/Bangalore

    I got this on a fresh install of 10.3.0.

  • First commit to issue fork.
  • Pipeline finished with Failed
    7 months ago
    Total: 184s
    #212590
  • Status changed to Needs review 7 months ago
  • 🇺🇸United States benjifisher Boston area

    From Comment #35:

    I applied the diff from 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work and the test here now passes. Maybe close this in favor of that?

    It looks to me as though the change from #3384600 that is relevant to this issue is out of scope for that issue. I suggest that we revert that change (+2/-2) in that issue and apply it here. I have updated the MR here with that change. The test in the MR now passes locally.

    One advantage to making the change as part of this issue is that it can be done in the next patch release. Since #3384600 adds a deprecation, it can only be done in the next minor.

    I am not sure what the difference is between findConfigEntityDependencies() and findConfigEntityDependenciesAsEntities(). I use the second, since that is what was used in #3384600, but I think the first is simpler and gives the same result here, since we extract the config dependency name from the entity/dependency object.

  • Pipeline finished with Failed
    7 months ago
    Total: 481s
    #212592
  • Pipeline finished with Failed
    7 months ago
    #212609
  • Pipeline finished with Success
    7 months ago
    Total: 520s
    #212896
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States benjifisher Boston area

    Tests are now passing, but NW for Berdir's comment on the MR.

    I can work on this after my day job, unless someone else wants to update the test.

    I have read most of the comments on this issue, and I think it is still a mystery why this bug seems to be more of a problem on 10.3 than on earlier versions. Has anyone checked the effect of the PHP version?

  • 🇩🇪Germany Anybody Porta Westfalica

    @Berdir thanks! I can only tell that we didn't switch the PHP version when upgrading to 10.3 where the issue appeared. So it's indeed still a mystery.

  • Pipeline finished with Success
    7 months ago
    Total: 575s
    #213319
  • Status changed to Needs review 7 months ago
  • 🇺🇸United States benjifisher Boston area

    I made the requested change to the test, so back to NR.

    This bug was first reported in 2022, with Drupal 9.4.5 and PHP 7.4. I have tested with 10.3.x and 10.2.0, and @quietone (Comment #4) used git bisect to determine that the bug was introduced with #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions . (I am adding that as a related issue.)

    Why, then, are people reporting that their logs are being flooded with this error after upgrading to 10.3.0?

    Here are some ideas. Probably some are silly.

    1. Something has changed in caching, so the access check for the Permissions form is not being cached effectively.
    2. Something has changed in a contrib module.
    3. Other errors and warnings have been fixed. This error has always come up a lot, but no one noticed it because of the noise.

    Maybe what we really need are better steps to reproduce (STR). The issue summary has STR a few warnings in the logs, but we need STR a flood of warning messages. Can we reproduce that with just Drupal core, or with just Drupal core and the admin_toolbar module? Does the behavior change when upgrading from Drupal 10.2.7 to 10.3.0?

  • Status changed to RTBC 7 months ago
  • 🇺🇸United States alison

    MR #8487 fixes the dblog warning flood for me on 10.3.0! (PHP 8.1, in case it matters) I don't see any side effects.

    (Setting to RTBC in hopes of getting it into 10.3.1 tomorrow -- apologies if this is bad form because it's not 11.x-dev as the issue is tagged.)

  • Status changed to Needs work 7 months ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • Pipeline finished with Success
    7 months ago
    Total: 865s
    #214437
  • 🇺🇸United States benjifisher Boston area

    @larowlan:

    Thanks for keeping me honest. I was being sloppy with the test.

    I spent some time just now looking at it. Among other things, I tried enabling the node module, adding a content type, and (with CommentTestTrait) adding a comment field to the content type. That was not enough to reproduce the error.

    @Berdir discussed the inconsistent configuration in Comments #16, #34, #41. I need more detail before I can figure out how to reproduce the problem without installing the standard profile. So I reverted my last commit, restoring the original test provided by @quietone.

    The test is slow: running locally, about 8s for the inconclusive test and 25s for the one hat installs the standard profile. So @Berdir's comment on the MR is valid. We should try not to install the standard profile in a test. But that is the best I can do for now.

    BTW, if you run the "Test-only changes" job in GitLab CI, expect EntityPermissionsFormTest to fail since the methods mocked in that test have to match the methods called in the code.

  • Status changed to Needs review 7 months ago
  • 🇺🇸United States benjifisher Boston area
  • 🇨🇭Switzerland berdir Switzerland

    Lets have a quick look before going to bed, I thought. That test shouldn't be too hard to resolve, I thought.

    I added all the obvious things, node type, comment type with a field through the two creation traits, a proper user with the necessary permissions, but still nothing.

    After a considerable amount of debugging, I tracked it down to an inconsistency between CommentDefaultFormatter (which defaults to view mode "default"), and addDefaultCommentField(), which defaults to the view mode "full". Combined with the unusual behavior of \Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::calculateDependencies() to only add a dependency on the view display if it exists. full didn't exist, so it couldn't load it, didn't add a dependency, then the node.article.default view display didn't depend on that and it didn't need to try and remove it from the dependency chain.

    I also added a few additional asserts, but the test is fragile by design and I don't see how we could change that. There is no positive outcome to look for here, the manage permissions tab isn't accessible, nothing must happen, that's the whole point of this issue. I did write it so that it will fail in 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work , so we can extend it a bit there and check the empty message and stuff instead of a 403 on the permission page. I do hope we still agree on doing that, the performance issues are at best partially resolved with this.

  • Pipeline finished with Failed
    7 months ago
    Total: 164s
    #215266
  • Pipeline finished with Success
    7 months ago
    Total: 563s
    #215391
  • Pipeline finished with Failed
    7 months ago
    Total: 691s
    #216038
  • 🇺🇸United States benjifisher Boston area

    @Berdir:

    Thanks for improving the test. I feel better that you agree it was not easy to do. In addition to being faster than installing the standard profile, your version shows that the view mode (default, not full) is involved.

    I made some minor changes to the test, including some changes to the comments.

    The test passes when I run it locally. If I revert the changes to EntityPermissionsForm, then it fails as expected:

    1) Drupal\Tests\system\Functional\Form\EntityViewDisplayCommentArticleTest::testError
    Behat\Mink\Exception\ResponseTextException: The text "Entity view display 'node.article.default': Component" appears in the text of this page, but it should not.
    

    Looking at the version of /admin/reports/dblog saved by the test, I see the full text of the warning message in the title attribute of the link:

    Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies.

    Personally, I am in favor of using named parameters, and this is a perfect example of when they are useful:

        $this->addDefaultCommentField('node', 'article', comment_view_mode: 'default');
    

    But I am not sure whether Drupal coding standards allow them. I think @alexpott pointed out that our BC policy allows us to rename parameters, which means we cannot rely on named parameters. On the other hand, this line is in a test, so if the parameter is renamed, the test will fail and be easy to fix.

    I cannot set the issue status to RTBC since I worked on this issue, but it looks good to me.

  • 🇨🇭Switzerland berdir Switzerland

    > But I am not sure whether Drupal coding standards allow them. I think @alexpott pointed out that our BC policy allows us to rename parameters, which means we cannot rely on named parameters. On the other hand, this line is in a test, so if the parameter is renamed, the test will fail and be easy to fix.

    Yes, I wasn't sure either, but it indeed felt like such a perfect use case for it, we'd need several arguments including long constants otherwise.

  • 🇨🇦Canada bsuttis

    I also hit this bug after upgrading to 10.3.0 and I found https://www.drupal.org/project/drupal/issues/3384600 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work to be closely related. I have an Article node type with a Comments field.

    I found that the main cause of the "Entity view display 'node.article.default': Component..." log was using getConfigEntitiesToChangeOnDependencyRemoval instead of findConfigEntityDependenciesAsEntities in
    EntityPermissionsForm::permissionsByProvider.

    After applying the MR patch here and running EntityViewDisplayCommentArticleTest the test passes locally for me. Reverting the MR patch, the test fails as expected due to the "Entity view display 'node.article.default" log.

    I also notice odd behaviour depending on the $comment_view_mode used in the test before the MR patch is applied.
    - 'default' calls CommentDefaultFormatter::calculateDependencies() 3 times and the "Entity view display 'node.article.default" log occurs (bad)
    - 'full' calls CommentDefaultFormatter::calculateDependencies() 1 time and the log does not occur (good)

    Is there is some specialness to 'default' that I haven't tracked down yet? The MR patch solves this too.

  • 🇨🇭Switzerland berdir Switzerland

    default vs full is explained in #61, CommentDefaultFormatter only adds a dependency if that config exists, otherwise it doesn't. This is inconsistent and weird, likely because of the described discrepancy between the default config and the test trait. But it's mostly a test-only thing, on a real site, you can't chose a view mode/display that doesn't exist as config, because only those are presented as options and it's not in scope of this issue.

  • Status changed to Needs work 6 months ago
  • 🇺🇸United States benjifisher Boston area

    I did some more investigation.

    First of all, I was under the impression that the warning message indicated some sort of problem with the configuration in the standard profile, perhaps a circular dependency. As @Berdir said in #41,

    I orig[i]nally thought that there’s an issue on how the config for comment formatter is defined, but no, everything is fine here.

    There is a problem with CommentTestTrait::addDefaultCommentField(): by default, it uses the view mode 'full' without defining it. That is why @Berdir’s test adds comment_view_mode: 'default'.

    Second, I realized that the warning is bogus. ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() has the optional parameter $dry_run, which defaults to TRUE. By default, that method clones the config entities (such as core.entity_view_display...) so that changes to them are harmless. The cloned entity is passed to ConfigManager::callOnDependencyRemoval() and then to EntityDisplayBase::onDependencyRemoval(). But that method does not know that the entity is cloned, so it logs a warning message when disabling a field.

    Third, I want to update the issue summary, and maybe make some tweaks to the tests, so I am setting the status to NW.

    Fourth, I figured out why this issue is more of a problem on 10.3 than on 10.2:

    Steps to reproduce

    1. Install Drupal with the standard profile.
    2. Add the Admin Toolbar module.
    3. Enable the admin_toolbar and admin_toolbar_tools modules.
    4. Log in as an admin user.
    5. Visit /admin/reports/dblog and reload the page a few times.

    With Drupal 10.2, nothing much happens. You can check that /admin/structure/comment/manage/comment/permissions is shown in the admin menu, even though you get a 403 response if you try to visit it.

    With Drupal 10.3, each time you load the page you get another copy of the warning message described in this issue. The permissions page is not shown in the admin menu.

    With some help from git bisect, I found that the change is caused by 🐛 [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed . I am adding that as a related issue. I confirmed that reverting the commit for that issue makes 10.3.x act like 10.2.x (as described above). There were some conflicts when I reverted the commit, but only in test classes.

  • Pipeline finished with Failed
    6 months ago
    Total: 169s
    #217649
  • Pipeline finished with Success
    6 months ago
    Total: 623s
    #217680
  • Status changed to Needs review 6 months ago
  • 🇺🇸United States benjifisher Boston area

    I decided to make some non-test changes as well. The biggest change is to use findConfigEntityDependencies() instead of findConfigEntityDependenciesAsEntities().

    I am updating the issue summary. I am removing the line

    Problem: Unfortunately, as a result, the Comment field is disabled when displaying Article Nodes (Hidden).

    I tested that, and did not find this problem. As I said in Comment #66, only a clone of the core.entity_view_display config object is modified.

  • 🇺🇸United States benjifisher Boston area

    I apologize for continuing to tweak, but I think the new test belongs in the user module. There is already a test class that covers per-bundle and per-module permissions forms, so I added it there.

  • Pipeline finished with Success
    6 months ago
    Total: 639s
    #218212
  • 🇨🇦Canada bsuttis

    Confirming @benjifisher's updates to the IS match the bug I encountered after the 10.3.0 upgrade and the latest MR passes testing locally for me.

    Thanks for the explanation in #66 too.

  • 🇺🇸United States adrianm6254

    An FYI, I applied MR8487 to 2 of my sites and the error is gone.

    Thanks!

  • 🇷🇴Romania andreic

    Same here, after applying the MR patch, the error is gone.

  • 🇨🇭Switzerland berdir Switzerland

    #66 and later all makes sense to me, the MR looks good to me as well. I wrote most of that test, so I don't think I can RTBC myself, but +1 from me.

    Plenty of people confirmed that it fixes their issue, all it takes now is someone confident enough to set it to RTBC ;)

  • 🇬🇧United Kingdom JamesOakley Kent, UK

    Meanwhile, while we wait for RTBC, please could someone link to the exact patch that works. I get that we're using a merge record for this, but for all my site deployments I'm simply using cweagans composer patches to apply any patches I need to core or contrib, and I'm not clear reading this history as to what patch I need to add to my composer.json file.

  • 🇨🇭Switzerland berdir Switzerland

    See https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... and make sure you read those warnings carefully. Do *not* use the URL directly in comnposer patches.

  • 🇮🇹Italy senzaesclusiva

    Same here on D 10.3.1 just upgraded from 10.2.x

  • 🇮🇹Italy senzaesclusiva

    [SOLVED] Well, i can confirm that this patch 3385600-5.patch 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work seem to solve the issue. No more system message on D 10.3.1, Php 8.3.8 related about Comment module or permissions

  • Status changed to RTBC 6 months ago
  • 🇬🇧United Kingdom catch

    This looks like a good fix for the problem. Moving to RTBC - would be good to get this into 11.0.0 as well the next 10..3 patch release.

  • 🇳🇿New Zealand quietone

    Regarding the question about named arguments. There is an open issue for that in the coding standards project, #3337834: Standards for PHP named arguments . Core has the following in the BC policy .

    Function arguments
    Named arguments should not be considered part of the public API.

  • 🇬🇧United Kingdom catch

    Trying a re-title.

    • larowlan committed 3f03a0aa on 11.x
      Issue #3306434 by benjifisher, quietone, Berdir, kopeboy, DYdave,...
    • larowlan committed c3c8f13a on 10.3.x
      Issue #3306434 by benjifisher, quietone, Berdir, kopeboy, DYdave,...
    • larowlan committed b3f0c378 on 10.4.x
      Issue #3306434 by benjifisher, quietone, Berdir, kopeboy, DYdave,...
    • larowlan committed de70a642 on 11.0.x
      Issue #3306434 by benjifisher, quietone, Berdir, kopeboy, DYdave,...
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Committed to 11.x and backported to 11.0.x, 10.4.x and 10.3.x

    Thanks everyone!

  • Status changed to Fixed 6 months ago
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024