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.
The new centralized entity loading is not consistently invoking hook_ENTITY_TYPE_load()
. hook_node_load()
works, but hook_comment_load()
doesn't work.
Comment | File | Size | Author |
---|---|---|---|
#21 | 569318_20_hook_comment_load_test.patch | 2.51 KB | marcingy |
Comments
Comment #1
scor CreditAttribution: scor commentedThis is needed by #493030: RDF #1: core RDF module.
Comment #2
mlncn CreditAttribution: mlncn commentedAnd the reason for hook_comment_load() not getting called is the CommentController does not include a call to the parent attachLoad function. This patch fixes that.
Comment #3
sunLooks about very right.
Comment #4
webchickOopsie.
Committed to HEAD. Thanks! We should have tests for this, too. Marking needs work for tests. Feel free to mark back to fixed if RDF will come with tests that test it already. Otherwise, this can get fixed sometime after this week.
Comment #5
alberto56 CreditAttribution: alberto56 commentedWe created a test that fails when the above patch is not applied, and which succeeds when the patch is applied. In addition to applying the patch, one must create the tests directory in modules/comments using the attached file. The reason is: we had to create a standalone module which implements hook_comment_load.
Comment #7
scor CreditAttribution: scor commentedzip files or not easy to review and cannot be tested by the bot. Please try to include all changes in the same patch using git or fakeadd.
how about adding the name of the hook in this messsage?
whitespace needs to be removed.
Comment #8
alberto56 CreditAttribution: alberto56 commentedHi, thanks for the comments, and the link to the git tutorial, that's exactly what I was looking for. Here is a new version of the patch.
Comment #9
alberto56 CreditAttribution: alberto56 commentedHere's a better version, sorry about that.
Comment #10
catchThis looks fine except all function names should be written
function()
instead offunction
in code comments, descriptions etc..Comment #11
alberto56 CreditAttribution: alberto56 commentedRight, missed that one. Here's an update.
A.
Comment #12
scor CreditAttribution: scor commentedminor tweak: make sure there is a new line at the end of all your files. You should never see
\ No newline at end of file
in a patch. otherwise looks good.Comment #13
alberto56 CreditAttribution: alberto56 commentedHi, thanks for your tips, I've added new lines. Here is a new version.
A.
Comment #14
alberto56 CreditAttribution: alberto56 commentedActually, use this one instead, I had forgotten a few ()s (as described in comment #10) -- this just might be about right...
Comment #15
scor CreditAttribution: scor commentedok, we're almost there. Please remove the whitespace right after the end of setUp(). You should have an empty line (see coding standards for Indenting and Whitespace).
Comment #16
alberto56 CreditAttribution: alberto56 commentedThanks for bearing with me, it's really appreciated! I'm not going to dare say this is the final one ~:)
Comment #17
scor CreditAttribution: scor commentedthanks to you alberto56 for bearing with the coding standards! It looks good to me now.
Comment #18
scor CreditAttribution: scor commentedrerolling. This patch only adds a test to
comment.test
. There is no code freeze for tests, is there? :)Comment #19
jeffreyd CreditAttribution: jeffreyd commentedBoth the patch and test work, but the patch is getting old (1 fuzz while patching, 131 lines offset).
Comment #20
marcingy CreditAttribution: marcingy commentedJust a reroll to head
Comment #21
marcingy CreditAttribution: marcingy commentedgrr windons
Comment #22
scor CreditAttribution: scor commentedback to RTBC
Comment #23
aspilicious CreditAttribution: aspilicious commented#21: 569318_20_hook_comment_load_test.patch queued for re-testing.
Comment #24
aspilicious CreditAttribution: aspilicious commented*Retesting old patches*
Comment #25
Dries CreditAttribution: Dries commentedIt seems like we'd want to make a class that tests more than one hook. Not?
Comment #26
Dries CreditAttribution: Dries commentedComment #27
scor CreditAttribution: scor commentedDries comment should be addressed by simply renaming the CommentCallHookLoad class to something more generic for testing hooks (ditto for module name). patch need reroll also.
Comment #28
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #38
apadernoComment #39
andypostThere's no such in 8.x core, so this issue is purely 7.x
Comment #40
apadernoQuite the opposite, Drupal 8 uses that hook, but the documentation shows it as
hook_ENTITY_TYPE_load()
.Comment #41
apaderno