Notices in comment_submit() and comment_admin_overview() when body field does not exist or is not required

it has some hard coding
related: #813052: Undefined index: format in comment_preview line 2065 and line 2183

not the same issue, I'm using 7.7, and also get the this issue after commet_body was deleted(the reason I deleted was it can't be retrieved by views, while fields can.):

Notice: Undefined property: stdClass::$comment_body 在 comment_submit() (行 2164 在 /opt/difang/prod/qlkaixin/modules/comment/comment.module).

fix for rogical

• disable comment module
• uninstall it using uninstall tab at modules list.
• open any sql browser and delete these tables:
  1. comment
  2. field_data_comment_body
  3. node_comment_statistics
  4. field_data_comment_body
  5. field_revision_comment_body
• enable comment module, et voila!

I hope it helps ;)

thanks, but it deleted all comment contents and more or less effect the core I think.

I mean my site is already on production.

Fixing issue metadata, it's wrong for these functions to assume the comment body exists. Should be an easy fix so tagging as 'novice'.

Back to normal, this is annoying but requires comments with no body which is relatively unusual.

would be usual when D7 users begin to be familar with comment entity.

and unfortunately views can't retrieve the default comment body, another body filed is needed.

+1
Working on a Drupal distribution that will have body field removed by default.

One more, the comment title would auto retrieve data from default comment body if the title is not filled.

This should be fixed if comment body is deleted. And as the comment body is designed to be deleteable, so this should be fixed.

not tested yet but make sense to me.

your patch is not for this issue, comment_body should not be hardcoded anymore as comment body may be removed.

@rogical

oh. do you meant we should use another field instead ? so if there have 2 fields, how to handle it ? Node hasn't this implement too.

there are 2 points:

1. the comment_body filed is deleteable, so there should no other errors if user delete it.

2. views can't retrieve comment_body, but OK to other fields(long text, text etc.), so if user want to use views to retrieve comment body, how to do that? The only solution is delete the default comment_body and create a long text field.

1. solved in patch
2. views problem, not a core issue.

back to review.

Subscribe.

@droplet: Please explain. If you are using $comment->comment_body without a check, how would that fix the issue?

+    $comment_body = $comment->comment_body[LANGUAGE_NONE][0]['value'];

Edit: As for the Views thing, I think you can display the comment body now.

same question, as $comment->comment_body should be have deleted.

I created a testcase to verify this bug; consider reviewing it and including it into your patches.
The secound patch is the test case + #10, though the patch is probably not the right one - just in case I missed something.

Setting to needs review to let the testbots have a go at it.

This won't be easy to fix properly, however. The point in #12 is right. We can't determine a good automatic title in all cases. Maybe we can in some?

Oh. my patch should not copy the $var again & the other errors show on #17 is easy to get rid of it.

Instead fix it, it can be totally remove comment body, not sure if committer like it.

If removed, we cannot backport to D7? Contri Modules are think it always exist there.

Patch attached gives someone who want to get rid of the error. I stay the issue status as need work for more discuss :)

My patch in #17 contained a random */, so the results didn't say a thing. However FAIL / FAIL was expected.

Corrected that. Now lets see how the testcase does on it's own. Hopefully it fails because of Notice: Undefined property: stdClass::$comment_body in comment_submit() (line 2183 of /home/user/public_html/drupal7/modules/comment/comment.module)..

And lets see how it does with your fix. So setting to needs review.

Reviewing your patch: It looks good.

One thing: Maybe leave

+      if ($comment->subject == '') {
+        $comment->subject = t('(No subject)');
+      }

out of the isset block?

Edit:
As for the Drupal 7 backport, I think we should do it. If the user deletes the body field, there should be no core errors. And of course no errors in Contrib as well, but we're not making it worse by fixing it in core.

@#20,
"out of the isset block?"

Your right, thanks.

Did that, another round of testing.

Great. RTBC.

Full disclosure: I wrote the testcase that verifies this bug and is part of the patch.

don't RTBC your own patches ;-)

Ok, thanks klausi. Wasn't sure since it wasn't my fix.

To me, the tests on this

+++ b/modules/comment/comment.admin.inc
@@ -98,13 +98,14 @@ function comment_admin_overview($form, &$form_state, $arg) {
+    $comment_attr_title = !empty($comment->comment_body[LANGUAGE_NONE][0]['value']) ? truncate_utf8($comment->comment_body[LANGUAGE_NONE][0]['value'], 128) : $comment->subject;

We don't usually use abbreviations in variable names ($comment_attr_title).

+++ b/modules/comment/comment.test
@@ -1977,4 +1977,21 @@ class CommentFieldsTest extends CommentHelperCase {
+  function testDeletedCommentBody() {
+    // Delete the comment body.
+    $this->drupalLogin($this->admin_user);
+    $this->drupalPost('admin/structure/types/manage/article/comment/fields/comment_body/delete', array(), t('Delete'));
+
+    // Preview an empty comment.
+    $this->drupalLogin($this->web_user);
+    $this->drupalPost('node/' . $this->node->nid, array(), t('Preview'));
+
+    // Post an empty comment.
+    $this->drupalPost('node/' . $this->node->nid, array(), t('Save'));
+  }

We don't assert anything here. Shouldn't we at least look for the new comment on the page?

26 days to next Drupal core point release.

+++ b/modules/comment/comment.module
@@ -2161,17 +2161,20 @@ function comment_submit($comment) {
     // Edge cases where the comment body is populated only by HTML tags will
     // require a default subject.
-    if ($comment->subject == '') {
+    if (trim($comment->subject) == '') {
       $comment->subject = t('(No subject)');
     }

Also, I'm not sure how this related to this bug report? Can you explain? I looks like a reasonable thing to do, but I think that should go in another issue.

26 days to next Drupal core point release.

Thank you for reviewing. I'll reroll the patch soon.
As for #27: Have a look at the hunk before that. A trim is removed there.

Here is a re-roll that also improves the tests by checking the administration overview page too.

I can confirm that the tests fail without the fix (due to the generated warnings), and the tests pass locally with the fix implemented. So all looks good to me.

I'll post a separate issue about the trim() in #27, since that's not related to what we are trying to fix here. We should keep fixes separate.

Thank you for rerolling.

I am a bit confused.

This line
$comment->subject = truncate_utf8(trim(decode_entities(strip_tags($comment_text))), 29, TRUE);
is moved into an if block. So isn't the missing trim a regression introduced by this fix?

I see! *stupid me*

Here is a new patch that correctly includes the trim we need if no comment_body exists (no new issue opened).

Where is the comment_body defined. I just tried to enter a new comment for a page and I got this error. I accidentally deleted the body on the page but restored it. Did that also delete the comment_body and if so how do I restore it?

Currently there is no way to restore the comment_body field in the UI in case it got deleted, as all custom fields are prefixed with field_ (except creating a new content type).

The patch here solves the warnings, but comment_body is still the hardcoded fieldname. I am not sure what we could do about that.

Actually there is a way to restore it ... add an EXISTING field ... not add a new field.

comment_body is not an existing field, as it's designed to be deleteable, so it should be able to be deleted!

I just the one want to delete the comment_body in my site, and use specified fields instead.

Oh, I wasn't aware of that. Apperently it works only if you have got at least one other content type where it exists - at least "on my machine". Maybe special fields like this one should always be in the "add existing fields" selectbox?

Then for now the workaround would be:
- Create a random content type
- Restore the comment_body field, because it's existing now
- Delete the random content type

Edit:
@rogical: Yes, but there is hardcoded behaviour that makes the field special. And one more thing is special: The name. That means (unless you use the crazy workaround) you can not restore that field, which again is special, because you can "restore" any other field. I'd love to make the comment_body field not special in any way - but that isn't a trivial problem.
If that problem isn't solved, there should at least be a way, to undo the deletion, if you need that special field for special behaviour.

Edit 2:
Maybe we could actually render the comment to get a few characters for the subject?

Edit 3:
Weren't there attempts to make the title field a normal field, too?

I was able to add the comment body field back to the comment fields and it is working without any error messages. Since the comment_body is a special field it is part of the core and can't be deleted (AFAIK) so adding it back is not a problem.

@Patricia_W: Please try this:
- Make sure you have only one content type.
- Delete the comment_body field.
- Try adding it back - that shouldn't be possible.

I hope you aren't serious about having only one content type. Do you mean I should remove all my content types? I do not intend to destroy my site.

Assuming you were on a testing/development site I was indeed serious ;) Luckily you didn't do that on production. Otherwise I would have felt guilty for not warning you.

So: It's good that this problem doesn't exists if you have content types where the comment_body field is still attached, but there is this edge case, where you can't add it back like that.

As I said ... I did add it back like that. I had also deleted the body field from the same content type while I was migrating to D7 and didn't know better, and I may have deleted the comment body at the same time. I had restored the body field by adding an existing field but had not noticed that that the comment_body was also missing until I tried to add a comment.

What do you mean by "edge case"?

What would cause the comment_body field not to be attached? Isn't it just part of the core fields?

That edge case is, again: There is no other content type that has a comment_body field. To reproduce that you would have to delete the content types or comment_body fields of your existing content types (Please don't do that on your production site). It is added by core code, but you can delete it field instances just like instances of any other field.

Thanks for the clarification. I did not read the fine print.

#32: 1038652-no-comment-body-field-2.patch queued for re-testing.

This error message also apears on comment preview page. It seems not so easy to fix, because it has a lot of hard coding.

Here seach results for "comment_body".

This is quite a confusing thread.
is there a way to fix the problem of getting:
Notice: Undefined property: stdClass::$comment_body in comment_submit() (line 2180 ...
in both the node when adding a comment and also in the admin comment page??

@wayne57: Yes, applying the patch in #29 should be a quick fix until this is polished enough to get in.

Admittedly I had forgotten about this issue. It shouldn't be too hard to get this fixed.

I am getting this in D7, will this work in D7 as well?

Yes. For this issue, however, we will have to do D8 first. Tagging novice to reroll the patch with LANGUAGE_NOT_SPECIFIED instead of LANGUAGE_NONE.

This reroll was a little complex due to all the changes that have occurred since last September. Hopefully I got it worked out correctly. We'll see what Testbot thinks.

The changes from LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED had already been made. I just had to roll them into the patch.

Oops, setting status.

#52 failed because CommentFormController::actions() tries to set the action elements' #weight based on #form['comment_body']['#weight'] without checking to see if it is set first.

I added the check and set the action elements' #weight = 100 by default if the body element doesn't exist. This doesn't seem like the best approach to me. I'm interested to know if anyone has a better suggestion.

I tested the functionality of the patch from #55. Verified that the error existed before the patch and that after cleanly applying the patch the error no longer appeared. Code review looks okay as well.

I guess this could use some more input on dcam's comments about setting a default #weight so I'm not going to change status atm.

#55: comment-1038652-55.patch queued for re-testing.

Marked #1371682: Undefined index: format in comment_preview line 2053 as a duplicate.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -294,17 +294,19 @@ class CommentFormController extends EntityFormController {
+      if (trim($comment->subject == '')) {

I'm pretty sure that this is not correct.

@dagmar: Yeah ... here's a reroll.

Hi

I'm kind of new, but I have the same issue here, instead of body_comment I use an image and I get the same error. Now, that patch sounds interesting but where do I apply it and how?

Thanks!
Tommy

#60: 1038652-no-comment-body-field-60.patch queued for re-testing.

I tried to reroll the patch, but it's clear a bunch of stuff has changed in the functions this patch affects. So I've removed the novice tag.

Rerolled the patch found in #60 against latest drupal-8.x in git; attaching both with and without actual fix to have a red and a green response from testbot. (However, it looks like the asserts mentioned in #26 have not been addressed yet; though the test result does show yellow exceptions. Hope that's enough for testbot to fail, too.)

#65: 1038652-no-comment-body-field-65-tests-and-fix.patch queued for re-testing.

Here is the fix for D7; tests are clean backport from D8 patch, fixes are mine (as I found no usable fix from the previous comments).

#68: 1038652-no-comment-body-field-68-tests-and-fix-D7.patch queued for re-testing.

Oh, forgot to remove the tag.

Setting the issue back to 8.x. The status is still Needs Review for the 8.x patch in #65.

I got this error trying to patch on D7.

$ git apply -v 1038652-no-comment-body-field-68-tests-and-fix-D7.patch
Checking patch modules/comment/comment.module...
error: modules/comment/comment.module: No such file or directory
Checking patch modules/comment/comment.test...
error: modules/comment/comment.test: No such file or directory

Rerolled #65 for latest 8.x.

74: 1038652-no-comment-body-field-74-tests-and-fix.patch queued for re-testing.

Instead of using isset()/empty, you could use $comment->hasField('comment_body').

Note that this will return TRUE for an empty field (I don't think that's actually allowed), unlike !empty().

Rerolled #74 against latest 8.x, following Berdir's advice from #77.

This issue/patch also resolves the problem of #2045409: Notice: Undefined offset: 0 in comment_submit() (line 2197 of comment/comment.module) :

I get the following notice when comment Body field is not required and not filled "Notice: Undefined offset: 0 in comment_submit() (line 2197 of mysite.com/modules/comment/comment.module)

I made it a duplicate of this issue.

But it then needs an extra testNotRequiredCommentBody()..

I applied #68 on 7.x successfully.
@errev (#74) : you could not apply the patch. It seems you started from the wrong directory.

Attached is a new D7-version of #68. It still lacks the extra testNotRequiredCommentBody() test.

Sending this back to 8.x.

Sorry to bother, attached D7-patches with tests.

The D8-patches from #78 are not valid anymore. Attached a new try.
- The fix contents did not need changing for 'not required comment_body'
- The test now contains an extra test for the new use case.

Not sure how to trigger the testbot for D8-patch.
[EDIT] Oh, seems something is happening...

OK, I learned how to run tests on my local machine, and how to write proper tests.
Attached D7-patches have better tests.
next stop: a fresh attack on D8.

Greetings, So do we wait for backport or do we keep tracking d7 issue (https://www.drupal.org/node/2077901 for example) ?

Not a problem in D8

Until there is a working patch for D7 and it's committed, I did the following to "solve" the problem:

• Restored the the original comment_body by using the "Add existing field" option
• Made the comment_body field required
• Added some placeholder text in the comment_body settings so that even if required, the user would not have to enter data into it
• Hid the comment_body field using Filed Permissions (https://www.drupal.org/project/field_permissions) Module

Indeed, the D8-version is very different, and has no problems.
I Added the symptoms of #2077901: Comment Preview Error for Plain Text (Undefined index: format in comment_preview()) to the summary.

@uwe_a, @wOOge, the best way to get this issue committed is to test the latest patch, confirm it works and set it to RTBC after 2 positive confirmations.

#100 solve problem for me. Thanks!

Looks like a testbot fluke.

Testbot fluke.

Testbot fluke.

I would like to get this in but there are a few issues here, mostly small things but not entirely.

1. +      if(isset($comment_text)){
   +        $comment->subject = truncate_utf8(trim(decode_entities(strip_tags($comment_text))), 29, TRUE);
   +      }
        }
   -    $comment->subject = truncate_utf8(trim(decode_entities(strip_tags($comment_text))), 29, TRUE);
   -    // Edge cases where the comment body is populated only by HTML tags will
   -    // require a default subject.
   +    // Edge cases where the comment body is populated only by HTML tags or does
   +    // not exist will require a default subject.
        if ($comment->subject == '') {
          $comment->subject = t('(No subject)');
        }
   

   Since we're no longer guaranteeing that $comment->subject will be trimmed, don't we have to do if (trim($comment->subject) == '') { at the bottom instead?

   Otherwise I think we're introducing a bug where the comment subject could wind up empty in the end, rather than saying "No subject".

2. +  /**
   +   * Test that saving an empty, required body generates a warning, and cannot be saved.
   +   */
   +  function testCommentEmptyRequiredBody($required = TRUE) {
   
   ....
   
   +  /**
   +   * Test that saving an empty, NON-required body generates a warning, and cannot be saved.
   +   */
   +  function testCommentEmptyNonRequiredBody($required = FALSE) {
   

   Why do these test functions take a parameter? There is no code that will ever pass it in...

3. +  /**
   +   * Test that a comment can be previewed and saved without the comment_body field,
   +   * and without generating warnings.
   +   */
   +  function testCommentDeletedBody() {
   

   The function summary should fit on one line, 80 characters or less.

4. +      elseif(isset($comment_body['value'])){
   +        $comment_text = check_plain($comment_body['value']);
   +      }
   

   Here and elsewhere, there are a lot of spacing issues (for example there should be spaces after "elseif" and before the "{" (see https://www.drupal.org/coding-standards#controlstruct).

Patch trying to address concerns in comment #119.

I patched with #120 and the notices went away.

I do confirm the patch in #120 is applied cleanly on PHP 7.0.8 and addresses the issue.

+ } // Attach the user and time information.
The comment should be on its own line, separate from the closing brace.

There are a bunch of trailing spaces.

With PHP 7.1.7 the last patch is giving:

git apply 1038652-comment-body-field-120-test-and-fix-D7.patch
1038652-comment-body-field-120-test-and-fix-D7.patch:61: trailing whitespace.
   * 
1038652-comment-body-field-120-test-and-fix-D7.patch:78: trailing whitespace.
  
1038652-comment-body-field-120-test-and-fix-D7.patch:81: trailing whitespace.
   * 
1038652-comment-body-field-120-test-and-fix-D7.patch:101: trailing whitespace.
   * Tests that the comment_body field is not required. 
1038652-comment-body-field-120-test-and-fix-D7.patch:102: trailing whitespace.
   * 
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.

Fixed the issues mentioned in #126 and #127.

#120 fixes the issues mentioned by David_Rothstein in #119. #128 is the same but without the whitespace errors.

#128 fixes the notice error for me too. Patch applies cleanly.

#128 does work for me too.

I wonder will this ever be applied?

The problem is urgent for v: 7.59

Unfortunatelly the patch #128 fails the core tests now. This needs work.