Problem/Motivation

Split from #2687977: Comment::preSave() unconditionally sets hostname to client host. A new comment entity gets a default value for an uninitialised hostname from the current request. But this is hardcoded in Comment::preSave().

Proposed resolution

Remove the hard-coding by creating an default value callback that sets the hostname from the current request.

Provide:

  • Tests
  • Update path.
  • Update path tests.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

The Comment hostname base field definition changes.

CommentFileSizeAuthor
#18 2860259-18.interdiff.txt988 bytesclaudiu.cristea
#18 2860259-18.patch6.22 KBclaudiu.cristea
#13 2860259-13.patch6.23 KBclaudiu.cristea
#13 2860259-13.interdiff.txt598 bytesclaudiu.cristea
#10 2860259-10.patch6.23 KBclaudiu.cristea
#10 2860259-10.interdiff.txt756 bytesclaudiu.cristea
#9 2860259-9.patch6.23 KBclaudiu.cristea
#9 2860259-9.interdiff.txt3.33 KBclaudiu.cristea
#8 2860259-7.patch5.95 KBclaudiu.cristea
#2 2860259-2.patch5.46 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs tests, -Needs upgrade path, -Needs upgrade path tests
FileSize
2860259-2.patch5.46 KB

Patch.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Title: Create a default value callback for Comment hostname base field » Move the comment hostname default value to a default value callback

Retitled.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented 
+      ->setDefaultValueCallback(static::class . '::getDefaultHostname');

Other calls to setDefaultValueCallback() done from Drupal core don't use static::class. Is there any reason not to use setDefaultValueCallback('Drupal\Comment\Entity\Comment::getDefaultHostname'), here?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented

@kiamlaluno, yes there is. The hardcoded namespace was used before PHP 5.5, when ::class was not introduced yet. At the beginning of Drupal 8 development cycle, PHP 5.4 was supported. But now, as the minimum PHP version is 5.5 and ::class has been introduced, this is a better way to refer a class fully namespaced name. See https://stackoverflow.com/questions/30770148/what-is-class-in-php to find out why this is better. See also http://php.net/manual/en/migration55.new-features.php#migration55.new-fe...

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
2860259-7.patch5.95 KB

Rerolled.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
2860259-9.interdiff.txt3.33 KB
2860259-9.patch6.23 KB

Moved the update test to functional tests.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
2860259-10.interdiff.txt756 bytes
2860259-10.patch6.23 KB

Oops!

The last submitted patch, 9: 2860259-9.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 10: 2860259-10.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Needs review
FileSize
2860259-13.interdiff.txt598 bytes
2860259-13.patch6.23 KB

ouch, sorry!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This makes a lot of sense and has test-coverage. LGTM.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2860259-13.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Needs work » Reviewed & tested by the community

Testbot had a hiccup re-rtbcing

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/comment/tests/src/Functional/CommentHostnameTest.php
@@ -0,0 +1,46 @@
+class CommentHostnameTest extends BrowserTestBase {

This should be possible via a kernel test right?

I don't see any navigation so I think we can move this to KTB

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
2860259-18.patch6.22 KB
2860259-18.interdiff.txt988 bytes

Sure.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC then. In #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface, I was wondering about the owner/uid, which currently must be set from the outside, which means it is not set at all for e.g. REST requests. But I guess that's better handled in a separate issue.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

updating review credit

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed 75850cd and pushed to 8.6.x.

  • larowlan committed 75850cd on 8.6.x 
    Issue #2860259 by claudiu.cristea, larowlan: Move the comment hostname...

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Related issue #2828793: Stop logging comment IP addresses by default