Comment body doesn't show up in preview for Anonymous users

The problem here is that comment field access is wrong. It checks entity access, but that's not the place to do that. Field access by definition assumes that you have access to the entity, so all that published and so on checking (which is what broke this) is unneeded and can be simplified a lot.

This adds tests for anonymous comment previewing which we didn't have any before.

this was dicovered during #2571909: CommentForm selects using the user formatted name

I think we should not modify the test cases like that. We should always test all fields for access, even if it is quite clear that view access to almost all of them is allowed. This is important for regression testing, when somebody tries to change comment field access logic later.

1. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -213,12 +213,7 @@ public function testAccessToAdministrativeFields() {
   -        $this->assertEqual($may_view, $set['user']->hasPermission('administer comments') || ($set['comment']->isPublished() && $set['user']->hasPermission('access comments')), SafeMarkup::format('User @user @state view field @field on comment @comment', [
   -          '@user' => $set['user']->getUsername(),
   -          '@state' => $may_update ? 'can' : 'cannot',
   -          '@comment' => $set['comment']->getSubject(),
   -          '@field' => $field,
   -        ]));
   +        $this->assertEqual($may_view, TRUE);
   

   hm, can we keep the nice message from above? Makes it easier to see what is going on when looking at the test results.

2. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -244,13 +239,11 @@ public function testAccessToAdministrativeFields() {
   -        $this->assertEqual($may_view, $field != 'hostname' && ($set['user']->hasPermission('administer comments') ||
   -            ($set['comment']->isPublished() && $set['user']->hasPermission('access comments'))), SafeMarkup::format('User @user @state view field @field on comment @comment', [
   -          '@user' => $set['user']->getUsername(),
   -          '@state' => $may_view ? 'can' : 'cannot',
   -          '@comment' => $set['comment']->getSubject(),
   -          '@field' => $field,
   -        ]));
   +        // Nobody has access to to view the hostname field and this case is
   ...
   +        if ($field !== 'hostname') {
   +          $this->assertEqual($may_view, TRUE);
   +        }
   

   same here. And I think we should have an else{} here that checks the hostname field right here instead of farther down.

3. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -266,13 +259,7 @@ public function testAccessToAdministrativeFields() {
   -        $this->assertEqual($may_view, $field != 'hostname' && ($set['user']->hasPermission('administer comments') ||
   -            ($set['comment']->isPublished() && $set['user']->hasPermission('access comments'))), SafeMarkup::format('User @user @state view field @field on comment @comment', [
   -          '@user' => $set['user']->getUsername(),
   -          '@state' => $may_view ? 'can' : 'cannot',
   -          '@comment' => $set['comment']->getSubject(),
   -          '@field' => $field,
   -        ]));
   +        $this->assertEqual($may_view, TRUE);
   

   and here

1. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -130,18 +130,14 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   +      // Nobody has access to the hostname. The mail field is hidden
   +      // from non-admins.
   

   Can we move the comment about mail down to the other if?

2. +++ b/core/modules/comment/src/Tests/CommentAnonymousTest.php
   @@ -40,6 +40,32 @@ function testAnonymous() {
   +    // Preview comments (with skip grant permission).
   

   skip grant permission? You mean skip comment approval permission?

3. +++ b/core/modules/comment/src/Tests/CommentAnonymousTest.php
   @@ -40,6 +40,32 @@ function testAnonymous() {
   +    $title = $this->randomMachineName(8);
   +    $body = $this->randomMachineName(16);
   

   Please, please no more randomness in tests :-( See #2571183: Deprecate random() usage in tests to avoid random test failures

   Let's replace this with hard coded example strings.

4. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -213,9 +213,8 @@ public function testAccessToAdministrativeFields() {
   +        $this->assertEqual($may_view, TRUE, SafeMarkup::format('User @user can view field @field on comment @comment', [
   

   we should use assertTrue()

5. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -244,13 +243,21 @@ public function testAccessToAdministrativeFields() {
   +        // Nobody has access to to view the hostname field.
   +        if ($field !== 'hostname') {
   +          $this->assertEqual($may_view, TRUE, SafeMarkup::format('User @user can view field @field on comment @comment', [
   +            '@user' => $set['user']->getUsername(),
   +            '@comment' => $set['comment']->getSubject(),
   +            '@field' => $field,
   +          ]));
   +        }
   +        else {
   +          $this->assertEqual($may_view, FALSE, SafeMarkup::format('User @user cannot view field @field on comment @comment', [
   +            '@user' => $set['user']->getUsername(),
   +            '@comment' => $set['comment']->getSubject(),
   +            '@field' => $field,
   +          ]));
   +        }
   

   Can we just have this long assert once and only set a $view_access variable to TRUE/FALSE within the if()?

6. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -266,10 +273,8 @@ public function testAccessToAdministrativeFields() {
   +        $this->assertEqual($may_view, TRUE, SafeMarkup::format('User @user can view field @field on comment @comment', [
   

   asserTrue()

Updated issue summary a bit.

The fix looks reasonable, I'll let @klausi approve the recent changes.

Looks good to me - thanks all

Looks like @klausi's feedback has been addressed, and subsystem maintainer signed off.

Committed and pushed to 8.0.x. Thanks!