Problem/Motivation

CommentAccessControlHandler checks for 'anonymous_contact' setting, it should be 'anonymous'

Proposed resolution

Fix

Remaining tasks

Review

User interface changes

None

API changes

None

Major b/c blocks #2227503: Apply formatters and widgets to Comment base fields

CommentFileSizeAuthor
#7 comment-access-busted.7.patch2.59 KBlarowlan
#7 interdiff.txt436 byteslarowlan
#4 comment-access-busted.pass_.patch2.45 KBlarowlan
#4 comment-access-busted.fail_.patch1.34 KBlarowlan
comment-access-busted.1.patch1.88 KBlarowlan

Comments

dawehner’s picture

dawehner
Primary language German
commented

Just loading a saving is a test coverage? This is not really obvious :(

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

No, i think it passes in head, so I need to work out why, but wanted an issue to point the comment base fields at (it is failing there)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#2227503: Apply formatters and widgets to Comment base fields
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
StatusFileSize
new comment-access-busted.fail_.patch1.34 KB
new comment-access-busted.pass_.patch2.45 KB

So the issue with the test was *all* combinations were testing MAYNOT_CONTACT, we need one to test MAY_CONTACT

Status: Needs review » Needs work

The last submitted patch, 4: comment-access-busted.pass_.patch, failed testing.

The last submitted patch, 4: comment-access-busted.fail_.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt436 bytes
new comment-access-busted.7.patch2.59 KB

Name isn't an administrative field, test was wrong - see CommentAccessControlHandler

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
grendzy’s picture

grendzy commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
@@ -135,7 +134,8 @@ public function testAccessToAdministrativeFields() {
-    $instance->settings['anonymous'] = COMMENT_ANONYMOUS_MAYNOT_CONTACT;
+    // Default is 'May not contact', for this field - they may contact.
+    $instance->settings['anonymous'] = COMMENT_ANONYMOUS_MAY_CONTACT;

Should we not be testing both situations?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Hi Alex,
Yes there are three other comments in the test which use the 'must not contact' option, the issue is none of the four were testing the 'may contact' option because I'd assumed the wrong default
Lee

alexpott’s picture

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

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 04fc787 and pushed to 8.0.x. Thanks!

  • alexpott committed 04fc787 on 8.0.x 
    Issue #2405691 by larowlan: CommentAccessControlHandler checks for an...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.