Problem/Motivation

When attempting to alter the render arrays of multiple entities using hook_entity_display_build_alter(), only the final entity in the $build_list array is actually altered. This is because the wrong array key is used in this line:

\Drupal::moduleHandler()->alter('entity_display_build', $build_list[$key], $context);

In #2450151: Don't try to render all fields (including hidden ones) for single entity display the variable name for the array key used in the loop where that line of code appears was changed, but the variable name was not changed on that particular line of code.

Proposed resolution

Patch attached, which includes the correction, and a test.

Remaining tasks

Needs community review.

User interface changes

None.

API changes

hook_entity_display_build_alter() will apply correctly to multiple entities passed in, not just to the last one.

Data model changes

None.

CommentFileSizeAuthor
#10 hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-10.patch3.89 KBdanmuzyka
#10 interdiff.txt2.27 KBdanmuzyka
#10 hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-10-TEST_ONLY.patch3.25 KBdanmuzyka
#5 hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-5-TESTS_ONLY.patch3.49 KBdanmuzyka
#5 interdiff.txt571 bytesdanmuzyka
#5 hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-5.patch4.14 KBdanmuzyka
#2 interdiff.txt270 bytesdanmuzyka
#2 hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-2.patch4.09 KBdanmuzyka
hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation.patch3.94 KBdanmuzyka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

danmuzyka created an issue. See original summary.

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
FileSize
hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-2.patch4.09 KB
interdiff.txt270 bytes

Adding another comment to the test in the patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

@danmuzyka could you attach the test-only patch to demonstrate the issue and fix?

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewHookTest.php
@@ -0,0 +1,58 @@
+ * @TODO: Add tests for these hooks:
+ * hook_entity_view_display_alter()
+ * hook_entity_prepare_view()
+ * hook_ENTITY_TYPE_view()
+ * hook_entity_view()
+ * hook_ENTITY_TYPE_view_alter()
+ * hook_entity_view_alter()

I think lowercase @todo is preferred, also without the :. Additionally, I don't think new @todos are allowed in core unless they link to an open issue, so adding that follow-up issue and adding the link to the code would be good.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 
+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewHookTest.php
@@ -0,0 +1,58 @@
+    // Confirm that the content added in entity_test_entity_display_build_alter()
+    // appears multiple times, not just for the final entity.

Nit: this exceeds the 80-character line-wrap limit by one :)

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
FileSize
hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-5.patch4.14 KB
interdiff.txt571 bytes
hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-5-TESTS_ONLY.patch3.49 KB

Attaching a modified version of the patch, and a patch with the test only, to demonstrate the failing condition.

Status: Needs review » Needs work

The last submitted patch, 5: hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-5-TESTS_ONLY.patch, failed testing.

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review

The TESTS_ONLY patch failed as expected.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Related issues: +#2755353: Add test coverage for entity view/render hooks

I think this is good to go. It addresses a clear bug, and demonstrates the issue with a test.

The follow-up issue in the @todo is #2755353: Add test coverage for entity view/render hooks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
  1. Great find! And great to see a followup in place for the other hooks.
  2. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -682,6 +683,17 @@ function entity_test_entity_prepare_view($entity_type, array $entities, array $d
+    $build['entity_display_build_alter']['#markup'] = Markup::create('Content added in hook_entity_display_build_alter');

    No need to use Markup::create() here - #markup will do automatic XSS filtering anyways and adding more usages for Markup::create() where it is not necessary could cause proliferation of unsafe practices. 

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewHookTest.php
@@ -0,0 +1,59 @@
+    // Confirm that the content added in
+    // entity_test_entity_display_build_alter() appears multiple times, not
+    // just for the final entity.
+    $this->assertNoUniqueText('Content added in hook_entity_display_build_alter');

    I would add the entity ID in the alter and assert each entity's customised version exists. It is a more explicit test.

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-10.patch3.89 KB
hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-10-TEST_ONLY.patch3.25 KB
interdiff.txt2.27 KB

Here's an updated version of the patch based on @alexpott's feedback.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

If you put the full patch last then the test-only patch won't result in the issue being set needs work by the bot. I've adjusted the weights but i don't think it is enough to stop the test above marking this needs work.

The last submitted patch, 10: hook_entity_display_build_alter-incorrect_array_key_passed_to_hook_invocation-2754783-10-TEST_ONLY.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Reviewed & tested by the community

I think this is ready to go! Thanks @danmuzyka!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 7c0a466ba66121489c7cf7889e3c7453327d2ed8 to 8.2.x and 41663bd to 8.1.x. Thanks!

  • alexpott committed 7c0a466 on 8.2.x 
    Issue #2754783 by danmuzyka, jhedstrom, alexpott:...

  • alexpott committed 41663bd on 8.1.x 
    Issue #2754783 by danmuzyka, jhedstrom, alexpott:...

  • alexpott committed 7c0a466 on 8.3.x 
    Issue #2754783 by danmuzyka, jhedstrom, alexpott:...

  • alexpott committed 7c0a466 on 8.3.x 
    Issue #2754783 by danmuzyka, jhedstrom, alexpott:...

Status: Fixed » Closed (fixed)

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