Problem/Motivation

Comment "body" field could be removed via field ui interface, so each comment trying to retrieve non-existent property (entity field api just silently returns NULL)

But when you delete a comment body field then you get notice in watchdog:

<em class="placeholder">Notice</em>: Trying to get property of non-object in <em class="placeholder">Drupal\comment\CommentForm-&gt;buildEntity()</em> (line <em class="placeholder">280</em> of <em class="placeholder">/home/andypost/www/core8/core/modules/comment/src/CommentForm.php</em>).

Proposed resolution

Skip additional processing when no comment body field.
(test only patch in #6)

Remaining tasks

review/commit

User interface changes

no

API changes

no

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because core assumes that comment always have "comment_body" field so throws warning
Issue priority Normal because just displays annoying warning
Disruption Non-disruptive
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

More clean-up:
1) use keyed array to not load each comment multiple times
2) comment author is rendered within build
3) use link template for edit form, content translation somehow not register template

PS: maybe change title to - revamp comment admin form

mgifford’s picture

So what are the best steps for testing this. It applies nicely.

andypost’s picture

Title: Comment admin screen should not expects comment body field exists » Comment module should check that comment body field exists
FileSize
2.04 KB
5.89 KB

There's no way to test that because \Drupal\Core\Entity\ContentEntityBase::__get cares to return null for unknown properties

But when you delete a comment body field then you get notice in watchdog

<em class="placeholder">Notice</em>: Trying to get property of non-object in <em class="placeholder">Drupal\comment\CommentForm-&gt;buildEntity()</em> (line <em class="placeholder">280</em> of <em class="placeholder">/home/andypost/www/core8/core/modules/comment/src/CommentForm.php</em>).

So I checked all core for other places, that included in patch

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Ok.. I reviewed the code. Looks fine. I applied it to SimplyTest.me, no problem. Added a article and a comment. All good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Surely we can write a test which create a comment type without a body field?

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.39 KB
1.68 KB
8.28 KB

Here's a test

andypost’s picture

Link related issue that found when writing a test

The last submitted patch, 6: 2422353-comment_body-6-fail.patch, failed testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc, test to post comment when no body field fails

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Why is the solution to this problem to check for an optional comment_body field versus just making comment_body required and unable to be removed from the UI? Like what does comment module actually do anymore without a comment body?

andypost’s picture

@webchick Personally I'm pretty sure that there's like 5% of usage for new comments to live without body. For example Event content type, with comments used to attach photo reports from the event.

Also we have a lot of troubles trying to prevent things (delete config entity) that supposed to be available. Here's what I'm following:
#1831928: Support a 'locked' property on ConfigEntities
#2147549: Prevent taxonomy_forums field be removed in admin interface
#2274433: Do not allow to alter Locked field via UI
#2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing

So I'm sure that better allow fault-tolerant solution then trying to limit core module functionality

andypost’s picture

Assigned: Unassigned » larowlan
FileSize
1.1 KB
7.79 KB

removed unnecessary changes to me more inline with current HEAD

@larowlan maybe separate admin overview fix (author is rendered) and what do you think overall on issue

larowlan’s picture

Assigned: larowlan » Unassigned

Other than the out of scope changes, this looks good to me

  1. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -183,18 +183,12 @@ public function buildForm(array $form, FormStateInterface $form_state, $type = '
    -      $username = array(
    -        '#theme' => 'username',
    -        '#account' => $comment->getOwner(),
    -      );
    
    @@ -204,7 +198,12 @@ public function buildForm(array $form, FormStateInterface $form_state, $type = '
    +        'author' => array(
    +          'data' => array(
    +            '#theme' => 'username',
    +            '#account' => $comment->getOwner(),
    +          )
    

    This seems out of scope, can we do that in a separate issue please?

  2. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -215,16 +214,16 @@ public function buildForm(array $form, FormStateInterface $form_state, $type = '
    -      $comment_uri_options = $comment->urlInfo()->getOptions();
    +      $comment_uri_options = $comment->urlInfo()->getOptions() + ['query' => $destination];
    ...
    -        'url' => Url::fromRoute('entity.comment.edit_form', ['comment' => $comment->id()], $comment_uri_options + ['query' => $destination]),
    +        'url' => $comment->urlInfo('edit-form', $comment_uri_options),
    ...
    -          'url' => Url::fromRoute('entity.comment.content_translation_overview', ['comment' => $comment->id()], $comment_uri_options + ['query' => $destination]),
    +          'url' => Url::fromRoute('entity.comment.content_translation_overview', ['comment' => $comment->id()], $comment_uri_options),
    

    Again, out of scope

andypost’s picture

Issue summary: View changes
FileSize
2.72 KB
5.76 KB

Added beta evaluation and removed unneeded changes #13 to separate issues:
#2505835: Optimize CommentAdminOverview
#2505841: Make CommentAdminOverview use link templates

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @andypost

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2422353-comment_body-14.patch, failed testing.

Status: Needs work » Needs review
jibran’s picture

Status: Needs review » Reviewed & tested by the community
andypost’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I agree that it is super hard to prevent deleting of configuration. Code should not rely on configurable fields.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 4eff3d1 and pushed to 8.0.x. Thanks!

  • alexpott committed 4eff3d1 on 8.0.x
    Issue #2422353 by andypost: Comment module should check that comment...

Status: Fixed » Closed (fixed)

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