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
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2860259-18.interdiff.txt | 988 bytes | claudiu.cristea |
#18 | 2860259-18.patch | 6.22 KB | claudiu.cristea |
Comments
Comment #2
claudiu.cristeaPatch.
Comment #3
claudiu.cristeaRetitled.
Comment #6
apadernoOther calls to
setDefaultValueCallback()
done from Drupal core don't usestatic::class
. Is there any reason not to usesetDefaultValueCallback('Drupal\Comment\Entity\Comment::getDefaultHostname')
, here?Comment #7
claudiu.cristea@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...Comment #8
claudiu.cristeaRerolled.
Comment #9
claudiu.cristeaMoved the update test to functional tests.
Comment #10
claudiu.cristeaOops!
Comment #13
claudiu.cristeaouch, sorry!
Comment #14
borisson_This makes a lot of sense and has test-coverage. LGTM.
Comment #16
alexpottTestbot had a hiccup re-rtbcing
Comment #17
larowlanThis should be possible via a kernel test right?
I don't see any navigation so I think we can move this to KTB
Comment #18
claudiu.cristeaSure.
Comment #19
BerdirBack 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.
Comment #20
larowlanupdating review credit
Comment #21
larowlanCommitted 75850cd and pushed to 8.6.x.
Comment #24
andypostRelated issue #2828793: Stop logging comment IP addresses by default