Update the default comment entity owner to the current user

This looks great! It does fix both bugs and it looks like a easy win because of the new trait. Great work @Sam152!

Looks good to me as well.

I was wondering if there's something to clean up thanks to this in e.g. CommentForm but the author/uid handling there is very complex, with weird empty fallbacks and so on (Node has similar stuff when it should simply be so that the field is required), so lets not touch that here.

The reason for removing the default value method is that we only just added it to 8.7.x only, so it's not an API yet.

Committed 0b8908a and pushed to 8.7.x. Thanks!

Also marked #2859812: Comments created with REST are assigned anonymous user despite of correct authenticication as a duplicate thanks to this :)

+++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
@@ -314,7 +314,7 @@ public function testPostDxWithoutCriticalBaseFields() {
     // DX: 422 when missing 'entity_type' field.
-    $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['field_name' => TRUE]), static::$format);
+    $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['entity_type' => TRUE]), static::$format);

I see what you did here, but actually, the comment was wrong, the title was right.

Looking at that method BEFORE this patch:

    // DX: 422 when missing 'entity_type' field.
    $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['entity_type' => TRUE]), static::$format);
    $response = $this->request('POST', $url, $request_options);
    // @todo Uncomment, remove next 3 lines in https://www.drupal.org/node/2820364.
    $this->assertSame(500, $response->getStatusCode());
    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
    $this->assertStringStartsWith('The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Symfony\Component\HttpKernel\Exception\HttpException</em>: Internal Server Error in <em class="placeholder">Drupal\rest\Plugin\rest\resource\EntityResource-&gt;post()</em>', (string) $response->getBody());
    // $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_type: This value should not be null.\n", $response);

[SNIP]

    // DX: 422 when missing 'field_name' field.
    $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['field_name' => TRUE]), static::$format);
    $response = $this->request('POST', $url, $request_options);
    // @todo Uncomment, remove next 2 lines in https://www.drupal.org/node/2820364.
    $this->assertSame(500, $response->getStatusCode());
    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
    // $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nfield_name: This value should not be null.\n", $response);

So the correct fix would've been to just change the comment :)

Also, as of this commit, JSON API's comment test coverage is failing. See #3001193: CommentTest::testPostIndividualDxWithoutCriticalBaseFields() fails on 8.7 since #2885809.

That test and its comments are very confusing because the broken functionality that it's asserting is very confusing. Completely understandable that you misinterpreted it!

Attached patch should be RTBC'able.

That 500 -> 201 change that also happens without any change there is why we assumed that this test coverage is no longer needed ;)

Looking at the actual exception that's causing the 500, that's happening in \Drupal\comment\CommentStatistics::update(), called by *Comment::postSave()*. Which means long after validation which was happy about this input.

Because of this line:

$this->entityManager->getStorage($comment->getCommentedEntityTypeId())->resetCache([$comment->getCommentedEntityId()]);

Because that translates to getStorage(NULL).

Everything else will create a lot of weird data in that comment statistics table but doesn't cause neither a hard fail *nor* a validation error.

And the reason this issue causes that change is that in \Drupal\comment\Plugin\Validation\Constraint\CommentNameConstraintValidator::validate(), we only check for getAnonymousContactDetailsSetting() which does indeed fail on having an empty entity_type but we only run that if owner_id === 0, which is now no longer the case now, so as a result it passes.

In other words, instead of solving the very hard to resolve #2820364: Entity + Field + Property validation constraints are processed in the incorrect order, we just need to make one single validator a tiny bit more resilient and mark the two field which should be required as required and we get exactly the DX that the test wishes to have for the comment entity :)

Just posting a quick patch that we should move to a dedicated issue. With these changes, it should pass with and without the patch from this issue. Maybe we need to postpone this on that, as this then shows that we are actually missing the required flag on that field and asserting a success condition here makes even less sense than asserting 500 exceptions :)

Needed one more check for having a field name there. Moved the patch to #2885809: The 'entity_type' and 'field_name' base fields on Comment are required.

[…] In other words, instead of solving the very hard to resolve #2820364: Entity + Field + Property validation constraints are processed in the incorrect order, we just need to make one single validator a tiny bit more resilient and mark […]

Splendid! But we'll still need to resolve #2820364: Entity + Field + Property validation constraints are processed in the incorrect order at some point.

Thanks for spinning this out to #2885809: The 'entity_type' and 'field_name' base fields on Comment are required. I RTBC'd your patch, but @amateescu felt it should work differently. Now leaving it to you to review that. It seems sensible to me, but I'm not as much as an expert in this area as you are :)

Marking this as postponed on #2885809: The 'entity_type' and 'field_name' base fields on Comment are required.

Just bumped the blocking issue.

That went in, so back to work here :)

Indeed! 🎉

Reroll of #23, without the change to the REST test.

Hi guys.

Are there any plans to have the latest patch merged to the core?

The issue is quite important since posting the comment via either rest or jsonapi causes to anonymous comment.

You're right, and this has been ready for ages — this just fell off the radar 😞

Re-testing against latest D8. Bumping priority.

#36 makes this sound like a bug. Re-categorising and updated issue summary to explain why this is not an API change. I was wondering whether this needs a change record but I think this changing comments to have the expected behaviour so I don't think so.

Committed and pushed 85fa773d51 to 9.0.x and 30c76f8e11 to 8.9.x. Thanks!

Going to ask other committers for a backport to 8.8.x

@catch +1'd the backport.

Great, thanks for the backport to 8.8!

#3083184: Drupal\comment\Entity\Comment::preCreate() should set the default value of `uid` to the current account ID was marked as a duplicate of this.