The new centralized entity loading is not consistently invoking hook_ENTITY_TYPE_load(). hook_node_load() works, but hook_comment_load() doesn't work.

CommentFileSizeAuthor
#21 569318_20_hook_comment_load_test.patch2.51 KBmarcingy
#20 569318_20_hook_comment_load_test.patch2.59 KBmarcingy
#18 569318_18_hook_comment_load_test.patch2.13 KBscor
#16 hook_comment_load.569318-16.patch2.13 KBalberto56
#14 hook_comment_load.569318-14.patch2.13 KBalberto56
#13 hook_comment_load.569318-13.patch2.12 KBalberto56
#11 hook_comment_load.569318-11.patch2.21 KBalberto56
#9 hook_comment_load.569318-9.patch2.2 KBalberto56
#8 hook_comment_load.569318-8.patch2.62 KBalberto56
#5 hook_comment_load.569318.patch1.49 KBalberto56
#5 tests.zip1.68 KBalberto56
#2 comment-fix-hook-load-569318-2.patch369 bytesmlncn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scor’s picture

scor CreditAttribution: scor commented
Issue tags: +RDF

This is needed by #493030: RDF #1: core RDF module.

mlncn’s picture

mlncn CreditAttribution: mlncn commented
Status: Active » Needs review
FileSize
comment-fix-hook-load-569318-2.patch369 bytes

And 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.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Looks about very right.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Oopsie.

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.

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
Status: Needs work » Needs review
Issue tags: +D7csmtl
FileSize
tests.zip1.68 KB
hook_comment_load.569318.patch1.49 KB

We 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.

Status: Needs review » Needs work

The last submitted patch, hook_comment_load.569318.patch, failed testing.

scor’s picture

scor CreditAttribution: scor commented

zip 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.

+++ modules/comment/comment.test	19 Feb 2010 18:57:14 -0000
@@ -1232,3 +1238,31 @@ class CommentContentRebuild extends Comm
+    $this->assertText('the hook was called');

how about adding the name of the hook in this messsage?

+++ modules/comment/comment.test	19 Feb 2010 18:57:14 -0000
@@ -1232,3 +1238,31 @@ class CommentContentRebuild extends Comm
+  ¶

whitespace needs to be removed.

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
Status: Needs work » Needs review
FileSize
hook_comment_load.569318-8.patch2.62 KB

Hi, 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.

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
FileSize
hook_comment_load.569318-9.patch2.2 KB

Here's a better version, sorry about that.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

This looks fine except all function names should be written function() instead of function in code comments, descriptions etc..

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
Status: Needs work » Needs review
FileSize
hook_comment_load.569318-11.patch2.21 KB

Right, missed that one. Here's an update.

A.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs review » Needs work

minor 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.

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
Status: Needs work » Needs review
FileSize
hook_comment_load.569318-13.patch2.12 KB

Hi, thanks for your tips, I've added new lines. Here is a new version.

A.

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
FileSize
hook_comment_load.569318-14.patch2.13 KB

Actually, use this one instead, I had forgotten a few ()s (as described in comment #10) -- this just might be about right...

scor’s picture

scor CreditAttribution: scor commented
Status: Needs review » Needs work 
+++ modules/comment/comment.test
@@ -1232,3 +1232,30 @@ class CommentContentRebuild extends CommentHelperCase {
+  function setUp() {
+    // Enable dummy module that implements hook_node_view.
+    parent::setUp();
+    module_enable(array('comment_hook_test'));
+  }
+  ¶

ok, 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).

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented
Status: Needs work » Needs review
FileSize
hook_comment_load.569318-16.patch2.13 KB

Thanks for bearing with me, it's really appreciated! I'm not going to dare say this is the final one ~:)

scor’s picture

scor CreditAttribution: scor commented
Title: hook_comment_load never gets called » add test for hook_comment_load never gets called
Status: Needs review » Reviewed & tested by the community

thanks to you alberto56 for bearing with the coding standards! It looks good to me now.

scor’s picture

scor CreditAttribution: scor commented
FileSize
569318_18_hook_comment_load_test.patch2.13 KB

rerolling. This patch only adds a test to comment.test. There is no code freeze for tests, is there? :)

jeffreyd’s picture

jeffreyd CreditAttribution: jeffreyd commented

Both the patch and test work, but the patch is getting old (1 fuzz while patching, 131 lines offset).

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Reviewed & tested by the community » Needs review
FileSize
569318_20_hook_comment_load_test.patch2.59 KB

Just a reroll to head

marcingy’s picture

marcingy CreditAttribution: marcingy commented
FileSize
569318_20_hook_comment_load_test.patch2.51 KB

grr windons

scor’s picture

scor CreditAttribution: scor commented
Status: Needs review » Reviewed & tested by the community

back to RTBC

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

#21: 569318_20_hook_comment_load_test.patch queued for re-testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

*Retesting old patches*

Dries’s picture

Dries CreditAttribution: Dries commented

It seems like we'd want to make a class that tests more than one hook. Not?

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work
scor’s picture

scor CreditAttribution: scor commented

Dries 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.

Tor Arne Thune’s picture

Tor Arne Thune CreditAttribution: Tor Arne Thune commented
Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +Needs backport to D7

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed a0ec577 on 8.3.x 
    #569318 by Benjamin Melançon: Fixed hook_comment_load() never gets...

  • webchick committed a0ec577 on 8.3.x 
    #569318 by Benjamin Melançon: Fixed hook_comment_load() never gets...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed a0ec577 on 8.4.x 
    #569318 by Benjamin Melançon: Fixed hook_comment_load() never gets...

  • webchick committed a0ec577 on 8.4.x 
    #569318 by Benjamin Melançon: Fixed hook_comment_load() never gets...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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
Version: 8.5.x-dev » 8.6.x-dev
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.6.x-dev » 7.x-dev
Issue tags: -Needs backport to D7 +Needs issue summary update

There's no such in 8.x core, so this issue is purely 7.x

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Version: 7.x-dev » 8.6.x-dev
Issue tags: -Needs issue summary update +Needs backport to D7

Quite the opposite, Drupal 8 uses that hook, but the documentation shows it as hook_ENTITY_TYPE_load().

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Issue summary: View changes

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • webchick committed a0ec577 on 9.1.x 
    #569318 by Benjamin Melançon: Fixed hook_comment_load() never gets...

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.