Problem/Motivation

These were forgotten during #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?") and uncovered in #2417895: AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it

From Wim Leers' comment at #2417895-45: AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it:

I looked at all remaining user.roles usages:

  1. BookNavigationBlock: wrong, is being fixed in #2483181: Make book navigation block cacheable.
  2. CommentDefaultFormatter: wrong, needed both user.permissions and user.roles, but can be updated to be more granular (use user.roles:authenticated, and only add it a level deeper in the if-branching)
  3. CommentForm: wrong, needed both user.permissions (always) and user.roles, but can be updated to be more granular (use user.roles:authenticated, and only add it a level deeper in the if-branching)
  4. history.module: used correctly
  5. LoginStatusCheck: used correctly
  6. \Drupal\user\Plugin\views\access\Role: used correctly
  7. RoleAccessCheck: used correctly

Opening an issue for fixing points 2 and 3 now.

This is that issue.

Proposed resolution

Review & commit.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#7 interdiff.txt664 byteswim leers
#7 2510034-7.patch1.81 KBwim leers
#4 2510034-4.patch1.18 KBwim leers
#3 2510034-3.patch1.77 KBwim leers
#1 2510034-1.patch3.01 KBwim leers

Issue fork drupal-2510034

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Issue tags: +Security improvements
StatusFileSize
new 2510034-1.patch3.01 KB
David_Rothstein’s picture

David_Rothstein commented 
+    if ($anonymous_contact != COMMENT_ANONYMOUS_MAYNOT_CONTACT) {
+      $form['#cache']['contexts'][] = 'user.roles:authenticated';
+      if (!$this->currentUser->isAuthenticated()) {
+        $form['#attached']['library'][] = 'core/drupal.form';
+        $form['#attributes']['data-user-info-from-browser'] = TRUE;
+      }

I think the user.roles:authenticated context may be needed unconditionally, not just in this if() statement? There are other places in the function that call $this->currentUser->isAuthenticated() and $this->currentUser->isAnonymous().

       if ($status == CommentItemInterface::OPEN && $comment_settings['form_location'] == CommentItemInterface::FORM_BELOW && $this->viewMode != 'print') {
         // Only show the add comment form if the user has permission.
-        $elements['#cache']['contexts'][] = 'user.roles';
+        $elements['#cache']['contexts'][] = 'user.permissions';

This will result in $elements['#cache']['contexts'] having multiple copies of 'user.permissions' (since it's also added earlier in the function under different, but often overlapping, conditions). I'm not sure if anything bad happens as a result of that, but ideally it would only appear in the array once.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Fix two wrong usages of the 'user.roles' cache context in Comment module » Fix one wrong usage of the 'user.roles' cache context in Comment module, and one extraneous use of the 'user.permissions' cache context
Priority: Normal » Minor
Issue tags: -Security improvements
StatusFileSize
new 2510034-3.patch1.77 KB

Rerolled. Most of this was fixed in #2543332: Auto-placeholdering for #lazy_builder without bubbling in the mean time.

#2:

  1. This hunk is no longer present now.
  2. There's no harm in doing so. But you're absolutely right that that is pointless. So, this patch rectifies that.
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Fix one wrong usage of the 'user.roles' cache context in Comment module, and one extraneous use of the 'user.permissions' cache context » Remove one useless cache contact in CommentDefaultFormatter
StatusFileSize
new 2510034-4.patch1.18 KB

The bulk of #3 was fixed in #2571909: CommentForm selects using the user formatted name. That leaves only the first hunk of #3 that still needs to be fixed.

Status: Needs review » Needs work

The last submitted patch, 4: 2510034-4.patch, failed testing.

The last submitted patch, 4: 2510034-4.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2510034-7.patch1.81 KB
new interdiff.txt664 bytes

Status: Needs review » Needs work

The last submitted patch, 7: 2510034-7.patch, failed testing.

Wim Leers queued 7: 2510034-7.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2510034-7.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

PIFR is no longer just drunk, it's also high. Not sure on what though.

Testbot maintainer Mixologic just told me to ignore PIFR results, only look at DrupalCI. PIFR is gonna be put to rest at the end of the week for D8!

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Remove one useless cache contact in CommentDefaultFormatter » Remove one useless cache context in CommentDefaultFormatter

Fixing title & retesting. This patch was ready and passing tests back in #7, more than a year ago!

Status: Needs review » Needs work

The last submitted patch, 7: 2510034-7.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +php-novice, +Needs reroll

Trivial test fail.

hussainweb’s picture

hussainweb
Primary language English
commented
Issue tags: -Needs reroll

The patch actually does not need a reroll. The tests probably need fixing though.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ivan berezhnov’s picture

ivan berezhnov commented
Issue tags: +CSKyiv18

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

useernamee made their first commit to this issue’s fork.

useernamee opened merge request !2075

useernamee’s picture

useernamee commented
Status: Needs work » Needs review
Issue tags: - +ddd2022, +Bug Smash Initiative

#3 Issue is still present in 9.3.x I've re-rolled the patch and opened merge request.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR needs to be updated to point to 10.1 and has a failure.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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.