Comment::preSave() unconditionally sets hostname to client host

The problem here is that the comment entity preSave() unconditionally sets the hostname to \Drupal::request()->getClientIP() - anything passed in by migration (or anything else creating comments programmatically) for hostname is simply ignored. I think preSave() should respect an explicit hostname, and default to getClientIP() when it's absent

Hey,

Something like this ?

Yep, just what I had in mind...

Tested patch against drupal 8.2.3 and works perfectly, thanks.

Given we need tests

Added tests and also a test-only patch.

I used for tests, documentation reserved IPs. See https://en.wikipedia.org/wiki/Reserved_IP_addresses

This part:

if (!$this->getHostname()) {
  // Ensure a client host from the current request.
  $this->setHostname(\Drupal::request()->getClientIP());
}

should be moved outside if ($this->isNew()) {...} to act also on updates. Otherwise parent comments, that are stubs during migration, will get an empty host and will never update.

IS update.

Only the "test only" failed, the main patch is green.

Nice, short fix. Good test coverage.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -151,8 +151,10 @@ public function preSave(EntityStorageInterface $storage) {
       }
-      // Add the values which aren't passed into the function.
       $this->setThread($thread);
+    }
+    if (!$this->getHostname()) {
+      // Ensure a client host from the current request.
...
     }
   }

Why are we moving this outside the if ...->isNew()? According to #10 it is because

Otherwise parent comments, that are stubs during migration, will get an empty host and will never update.

Is this tested?

@alexpott, yes is tested in the D6 test where we have a parent comment. The test revealed me this problem. Without the fix, comment with cid 3 will return an empty hostname. This is because, that one being parent, is created first as stub, so the hostname is initially set to NULL. Then, when the stub is updating and is receiving a valid hostname, that part is not triggered because, this time, $this->isNew() === FALSE.

Given this is data loss during migration it is at least a major. I'm not sure this is critical data though.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -151,8 +151,10 @@ public function preSave(EntityStorageInterface $storage) {
-      // Add the values which aren't passed into the function.
       $this->setThread($thread);
+    }
+    if (!$this->getHostname()) {
+      // Ensure a client host from the current request.
       $this->setHostname(\Drupal::request()->getClientIP());
     }

But if I move this the kernel test doesn't fail :(

Also it doesn't make sense because if a stub is created then it'll have the host set to \Drupal::request()->getClientIP().

@alexpott, well, yes. I cannot reproduce anymore the situation where I got an empty IP for cid 3. I'm fixing the IS.

@Jo Fitzgerald, are you so kind to re-review if turns green?

Wondering if a better fix would actually be to make this a default value callback instead of setting it on preSave(). But that's a bigger change.

@Berdir yeah - maybe a follow-up. Do the simple fix for the data integrity issue here and do it right in an 8.4.x followup.

@alexpott, @Berdir, opened #2860259: Move the comment hostname default value to a default value callback.

We have a green patch also here #2860259: Move the comment hostname default value to a default value callback. Needs review.

Bearing in mind the follow-up, I think this simple fix can now be set as RTBC.

Committed and pushed 012bca1 to 8.4.x and b1642ec to 8.3.x. Thanks!