Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When migrating Drupal 6/7 comments the hostname value is not migrated correctly to Drupal 8. IP-addresses in the D6 comments.hostname column should go to comment_field_data.hostname in D8. But the destination values are all set to the IP of the localhost (127.0.0.1). Other values, such as creation timestamp and mail are migrated in good order.
Proposed resolution
Only assign current request client IP when no hostname has been previously set.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#17 | 2687977-17.patch | 4.37 KB | alexpott |
#17 | 10-17-interdiff.txt | 769 bytes | alexpott |
#10 | 2687977-10-test-only.patch | 3.57 KB | claudiu.cristea |
#10 | 2687977-10.patch | 4.33 KB | claudiu.cristea |
Comments
Comment #2
mikeryanThe 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
Comment #3
rosinegrean CreditAttribution: rosinegrean as a volunteer commentedHey,
Something like this ?
Comment #4
mikeryanYep, just what I had in mind...
Comment #7
edemus CreditAttribution: edemus commentedTested patch against drupal 8.2.3 and works perfectly, thanks.
Comment #8
dawehnerGiven we need tests
Comment #10
claudiu.cristeaAdded 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:
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.Comment #11
claudiu.cristeaIS update.
Comment #13
claudiu.cristeaOnly the "test only" failed, the main patch is green.
Comment #14
jofitz CreditAttribution: jofitz at ComputerMinds commentedNice, short fix. Good test coverage.
Comment #15
alexpottWhy are we moving this outside the
if ...->isNew()
? According to #10 it is becauseIs this tested?
Comment #16
claudiu.cristea@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
.Comment #17
alexpottGiven this is data loss during migration it is at least a major. I'm not sure this is critical data though.
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()
.Comment #18
claudiu.cristea@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?
Comment #19
BerdirWondering 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.
Comment #20
alexpott@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.
Comment #21
claudiu.cristea@alexpott, @Berdir, opened #2860259: Move the comment hostname default value to a default value callback.
Comment #22
claudiu.cristeaWe have a green patch also here #2860259: Move the comment hostname default value to a default value callback. Needs review.
Comment #23
claudiu.cristeaComment #24
jofitz CreditAttribution: jofitz at ComputerMinds commentedBearing in mind the follow-up, I think this simple fix can now be set as RTBC.
Comment #25
alexpottCommitted and pushed 012bca1 to 8.4.x and b1642ec to 8.3.x. Thanks!