🇺🇸United States @benjifisher

The second MR for 10.3.x still needs updates.

I agree that we do not need additional functional tests.

According to https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → , we do need

A unit test proving the deprecation notice will be triggered when the deprecated code is called. Use a @group legacy annotation in conjunction with calls to

$this->expectDeprecation()

Back to NW for that.

benjifisher → created an issue.

I was going to update the status of this issue to NR, but then I saw that there is a failing test. It is the one we added in 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active . We knew we would have to update that test, but we forgot to do it. (See Comment #34 here.)

When we actually remove the access function, I think we can remove some parts of the functional test. Or maybe we should do it now, since the pages we are testing do not (after the changes here) use the access function.

I can review the code, which is the R in RTBC. But someone else will have to test (T) since I do not have a site that is having performance issues. See Comment #38 for the sort of detail we need. It would be good to get some performance data for

1. Drupal 10.3.0 or 10.3.1.
2. Drupal 10.3.x, since #3306434 is now Fixed.
3. Drupal 10.3.x with the changes here, or the branch from the MR

.

@Summit, I will repeat the advice that @Berdir gave in #3306434-75: Fix access checks for bundle permissions to avoid triggering a config validation error → :

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

Drupal 10.3.2 should be released on 2024-08-07. As I said above, this issue needs to be tested first. If that is done soon, then there is a good chance that this issue will be part of the 10.3.2 release. (See https://www.drupal.org/about/core/policies/core-release-cycles/schedule#... → .)

benjifisher → created an issue.

I am adding a note to the issue summary about the patch for 10.2.x.

Now that 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active is fixed, we can un-postpone this issue.

We need remove the changes in the MRs that were already made by #3306434. I left comments on the MRs.

I am curious how much #3306434 helps the performance, if at all. Should this issue still be prioritized as Major?

bbrala → credited benjifisher → .

We skipped the 2024-07-04 meeting, so I am re-purposing this issue for the 2024-07-18 video meeting,

benjifisher → created an issue.

Thanks for linking to the related issues.

Is the idea to only do this in syslog because dblog is ephemeral?

I admit I did not give it a lot of thought. I am setting up syslog for my current project, and I asked whether we should be logging IP addresses. I was told not to. If that is common advice, then we should change the default.

Also, changing the default for syslog is easy. Adding an option to the dblog module would be more work.

I am not opposed to expanding the scope of this issue to include dblog.

This data is often useful, ..., to me this is won't fix.

You may be right. Either way, it is easy to change the default. We just have to decide which use case is more common, and I think it makes sense to err on the side of less sophisticated site owners.

benjifisher → created an issue.

We might also talk about #3201550: Add new “Content Manager” role to Standard Profile → . @rkoller and I discussed it in Slack a few days ago. I think we can use recipes to achieve the goal of that issue.

benjifisher → created an issue.

The subsystem maintainers all agree on this plan, so I am removing the tag for that.

benjifisher → created an issue.

+1 for the list of attendees.

benjifisher → created an issue.

I am restoring the issue summary. It seems that this issue was updated by a spammer, whose account is already blocked.

@catch, thanks for the references in Comment #28.

If we agree to handle the error message in #3306434, then we can update the MRs for this issue and they can be fixed in either order.

We decided to skip the 2024-06-27 meeting since so many people were busy at Drupal Dev Days. I am repurposing this issue for the next meeting.

I wrote,

Since this issue introduces a deprecation, it might not be eligible for a patch release.

On second thought, we might deprecate the slow route provider in Drupal 11.x and 10.4.x, then have a separate MR for 10.3.x that does not include the deprecation but does switch the route provider for comment_type and contact_form.

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.

It looks as though we will replace the call to getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() or findConfigEntityDependencies() in 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active . If so, then we will have to update (simplify) the MR for this issue.

Since this issue introduces a deprecation, it might not be eligible for a patch release. As a practical matter, I think it makes sense to implement hook_entity_type_alter() in the admin_toolbar module. I opened 📌 Bypass slow access checks Needs review to do that, with a variant of @larowlan's code sample in #3306434-14: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. → .

The issue summary here mentions "multi-second page loads with admin_toolbar module and etc.", and Comment #9 says, "this is causing WSOD on some sites." I have not seen that. Can someone provide steps to reproduce?

+1 for the technical analysis in Comment #25. Unless we add a new API, the only reliable way to get the permissions related to a bundle is to use the config dependency system, and that is inherently slow.

I will be disappointed if we give up the per-bundle permissions forms that we added in Drupal 9.4. At that time, it seemed like a good idea not to show links to forms with no permissions, and access control is the only way I could think of to implement that. I will not try to convince anyone not to remove those checks (this issue) but I will not actively support it until I know how to reproduce the serious problems. (I will also suggest alternative fixes.)

I opened MR 82, implementing the proposed resolution. The code is based on @larowlan's comment #3306434-14: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. → .

It might be a good idea to improve this by making the affected types configurable. Then site administrators who know that there are no permissions to manage could completely remove those forms (unset($route_providers['permissions']); in the hook implementation); those who know there are permissions to manage could access them without the slow check.

benjifisher → created an issue.

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.

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.

The reason the patch does not apply is likely that the patch is for Drupal 11.x.

That is true. Specifically, most plugins are converted from annotations to attributes in 10.3.x and 11.x. Even more specifically: layout plugins use annotations in Drupal 10.2, and they use attributes in 10.3 and 11. That is why the patch does not apply.

The patch https://www.drupal.org/files/issues/2024-04-02/3392572-6533.patch → from Comment #61 should work for Drupal 10.1. I just checked, and it applies cleanly to 10.2.x and 10.2.7. It was hidden in Comment #71, and I am un-hiding it now, so it will show up on the list of files attached to this issue.

benjifisher → created an issue.

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

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

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?

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?

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.

benjifisher → made their first commit to this issue’s fork.

I think that replacing getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() is a good idea, but I do not see how it is in scope for this issue.

If that change is in scope, then it should be explained under "Proposed resolution" in the issue summary.

I think it is more likely that the change is out of scope. In fact, according to Comment #11, that change is likely to fix 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active . (I did a quick check, and this seems correct.) If so, then let's make the change on that issue and get it into the next patch release. This issue introduces a deprecation, so it cannot be made until the next minor release.

In my previous comment, I suggested that, instead of deprecating EntityPermissionsRouteProviderWithCheck in Drupal core, we should implement hook_entity_type_alter() in the admin_toolbar module. I am adding an item to "Remaining tasks" to decide which approach to use.

I do not see how the two issues are related, but 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active is mentioned here (Comments #11, #12, #14), so I am adding it as a related issue. If I read the comments correctly, that issue will be fixed by the change here.

I am fixing a typo ("Provicer" should be "Provider") here and in the change record (CR). I am also correcting the title of the CR.

I am also filling out the "User interface changes" and "API changes" sections of the issue summary. If that is enough, then we can remove the "Needs issue summary update" issue tag.

I am also adding a work-around to the issue summary. Has anyone considered adding that work-around to the admin_toolbar module instead of removing the EntityPermissionsRouteProviderWithCheck class from core? Are there problems from this route provider on sites that do not use admin_toolbar?

benjifisher → created an issue.

benjifisher → created an issue.

Here is a more complicated example from my current project. A custom source plugin provides the source field filters that looks something like this:

[
  [
    'vid' => 'some_vocab',
    'tids' => [1, 2, 3, 5],
  ],
  [
    'vid' => 'another_vocab',
    'tids' => [8, 13, 21],
  ],
]

That represents two vocabularies and a few terms from each vocabulary.

Here is the pipeline:

  field_hwp_default_filter_values:
    - plugin: sub_process
      source: filters
      process:
        data:
          - plugin: array_template
            template:
              target_id: 'pipeline:'
            source: tids
        reference_field:
          - plugin: migration_lookup
            migration: hwp_vocabularies
            source: vid
          - plugin: array_template
            template:
              - field
              - 'pipeline:'
          - plugin: concat
            delimiter: _
    - plugin: single_value
    - plugin: array_template
      template:
        - 'pipeline:'
        - data
        - reference_field
    - plugin: callback
      callable: array_column
      unpack_source: true
    - plugin: callback
      callable: serialize

After the first step in the pipeline (sub_process), the example at the top is converted to this:

[
  [
    'data' => [['target_id' => 1], ['target_id' => 2], ['target_id' => 3], ['target_id' => 5]],
    'reference_field' => 'field_some_vocab',
  ],
  [
    'data' => [['target_id' => 8], ['target_id' => 13], ['target_id' => 21]],
    'reference_field' => 'field_another_vocab',
  ],
]

By default, a process plugin (like array_template) is applied to each element of a source array. In this example, migration_lookup is a no-op.

The single_value process plugin overrides that default behavior, so the next array_template prepares the input for the callback plugin:

[
  [['data' => ..., 'reference_field' => ...], ['data' => ..., 'reference_field' => ...]],
  'data',
  'reference_field',
]

and so the callback plugin returns

array_column(..., 'data', 'reference_field')

or

[
  'field_some_vocab' => [['target_id' => 1], ['target_id' => 2], ['target_id' => 3], ['target_id' => 5]],
  'field_another_vocab' => [['target_id' => 8], ['target_id' => 13], ['target_id' => 21]],
]

The last step in the pipeline uses callback with callable: serialize to serialize that array.

benjifisher → created an issue.

benjifisher → created an issue.

I changed the plugin ID to array_template, and I changed the class names (plugin and test classes) to match.

I see that @rkoller and @zetagraph have commented on 📌 Improve the categories filter type in context to the rest of the filter component ui Needs work since the meeting, so I am marking that step as done even though none of those comments is a formal usability review.

benjifisher → created an issue.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-06-07 Needs work . That issue has a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @SKAUGHT, @benjifisher, @rkoller, @shaal, @simohell, @worldlinemine, and @zetagraph.

We tested the patch from Comment #2, and we have four recommendations:

1. Instead of adding the text, "disabled by parent", add a new column to the table. We suggest the column header "Visible", between the "Enabled" and "Operations" columns, and each row should be "Yes" or "No". If a menu link is disabled, or if any of its parents are disabled, then the new column should be "No".
2. Add a CSS class to the table row (<tr> element) with the same information. Then the admin theme has the option of using a background color as another way to indicate the state.
3. When a menu item is moved to a different parent, the new column should be updated. For example, if the original parent is disabled and the menu item is moved under an enabled parent, then the new column should update from "No" to "Yes" immediately, not when the form is submitted. The CSS class from (2) should also be updated.
4. Add the same information to the edit page for the menu item, like /admin/structure/menu/link/entity.entity_view_mode.collection/edit (for the "View modes" link in the Structure menu).

The reason for (1) is that the current solution creates too much "visual noise". The feature is most useful when the disabled menu item has a lot of children. (If it has only 2 or 3 children, then it is usually easy to see the parent item when looking at the children.) But "a lot of children" implies a lot of repetitive "(disabled by parent)" notices. We also feel that it will be easier to scan if the information is in a separate column. Since the menu titles have different lengths, the notices in the current solution do not line up.

We are not sure we like the current "(disabled)" notice. It duplicates the information in the Enabled column and is not easy to scan. But we are not suggesting that you change that: it is out of scope for this issue.

If you disagree with (1), then it would help to implement it anyway. Once we see it, we may be convinced that it is not a good idea after all.

We considered a few ways to implement (4). If we end up with the current solution instead of (1), then it would be consistent to add the same information ("disabled by parent") to the "Parent link" select list. Or we could add something to the help text (description) of the "Enable menu link" checkbox. But we prefer a third way: if the selected Parent link (or one of its ancestors) is disabled, then add a new text element below the "Enable menu link" checkbox. We did not discuss the exact wording, but something like this should work:

This menu link will not be shown because one of its ancestors in the menu tree is disabled.

If we can say which ancestor is disabled, then I think that will be even better.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

benjifisher → created an issue.

benjifisher → created an issue.

I commented on the two issues that we discussed today.

@smustgrave:

When you tag an issue for usability review, please make it easy for the usability team to review the issue. Update the issue summary:

• The "Proposed resolution" section should describe all the changes made in the issue.
• The "User interface changes" should show the existing UI and the proposed UI.
• The "Remaining tasks" should include one explaining the usability issue(s).

Most of the time, I prefer to have plain text in the "Proposed resolution" section and screenshots in the "User interface changes" section. For this issue, (1) needs work (already noted in the "Remaining tasks"), (2) is already in good shape, and (3) is missing.

You can also attend the weekly usability meeting to present an issue.

I am setting the status to NW.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-05-31 Needs work . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @simohell, @shaal, and @SKAUGHT.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

The only usability issue we see in the comments is from Comment #140:

My only concern is this feels odd. You essentially have two full text fields now so why not add 2 fields.

As far as we can tell, that is out of scope for this issue. It is certainly relevant for 🌱 [Meta] Deprecate text_with_summary Active , but in the context of this issue, we have two text fields and they both use the same text format. It makes sense to use CKEditor for both of them.

We also discussed whether it was worth the effort to discuss this issue, considering #3427095. We decided it is worth our time: if the text_with_summary is deprecated in Drupal 11 and removed in Drupal 12, that means it will still be around for several years.

We only tested the updated code on the Body field during the meeting, and I just noticed Comments #144, #145. Personally, I agree: if we are going to make this change, then it should apply to all text_with_summary fields, not just the body field.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-05-31 Needs work . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @simohell, @shaal, and @SKAUGHT.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

We agree that having more specific error messages is an improvement, and I am adding the Usability tag. The suggested messages are clear. We think it might help to come up with a shorter version of these two:

• "The path '@uri' is external, but the link field only supports internal paths."
• "The path '@uri' is internal, but the link field only supports external paths."

We do not have a firm recommendation, but it might help to break each one into two, short sentences. For example, the second might be "The path '@uri' is internal. This field only supports external paths." Can we use the field label instead of "This field"?

I am also adding the tag for an issue summary update:

• Under "Proposed resolution", please add each of the new messages, along with steps to generate each one. For example, I am not sure how to trigger the message, "The path '@link_path' has an invalid parameter."
• Under "User interface changes", add at least one pair of before/after screenshots. We need screenshots for all of the new messages

.

I also created a merge request based on the patch in #34. I edited some context lines in the patch so that it applies to the current 11.x branch.

benjifisher → made their first commit to this issue’s fork.

benjifisher → changed the visibility of the branch 10.3.x to hidden.

Back to NR for https://git.drupalcode.org/project/drupal/-/merge_requests/8250

The deprecated code is already removed in the 11.x branch, so this follow-up is needed on the 10.3.x branch only.

benjifisher → changed the visibility of the branch 3303557-reference-change-record to hidden.

I reviewed the MR, and it does just what the change record says. (Thanks for adding a link to the change record in the issue summary.) I also searched for other uses of 'method' => 'replace',, and I did not find any.

The hard part is testing. From the issue description:

Both methods work identically before Drupal 10.3.0, but starting in this version, only replaceWith is supported.

Not quite. Starting with Drupal 10.3.0, replace generates a deprecation notice, which is not easy to test. I did it by hacking the line

        @trigger_error('Using "replace" as the method in #ajax property is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use "replaceWith" instead. See https://www.drupal.org/project/drupal/issues/3303557', E_USER_DEPRECATED);

in Drupal\Core\Render\Element\RenderElementBase::preRenderAjaxForm(), removing the @ so that the error is not suppressed. After that change, I see a deprecation notice (debug level) in the logs each time I load the page at /admin/config/system/shield.

When I make the change in the MR, I do not see any log messages when loading that page.

Is it worth making a follow-up MR to reference the change record, not this issue, in the deprecation message?

benjifisher → created an issue.

+1 for RTBC

Just three of us attended the meeting just now. I am adding credit to this issue.

benjifisher → created an issue.