Problem/Motivation

In https://www.drupal.org/node/2916667 a standardised way to implement computed fields has been introduced. However, if you create an entity reference computed field, whose field item list class extends the EntityReferenceFieldItemList class, you cannot use the ::referencedEntities() method. This is proved by the attached test.

Proposed resolution

  • Use ::isEmpty() in ::referencedEntities() to precompute the values.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#9 2978848-9.patch4.52 KBclaudiu.cristea
#9 2978848-9.interdiff.txt2.13 KBclaudiu.cristea
#5 entityreference-computed-fail.patch4.08 KBclaudiu.cristea
#2 2978848-2.interdiff.txt1.6 KBclaudiu.cristea
#2 2978848-2.patch4.73 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 for European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
2978848-2.patch4.73 KB
2978848-2.interdiff.txt1.6 KB

The fix.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Issue tags: +Needs change record updates
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Parent issue: » #2392845: Add a trait to standardize handling of computed item lists
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
entityreference-computed-fail.patch4.08 KB

Ouch... the failing patch

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

The test-only patch from #5 looks very good, but I'm not sure about the fix. The proposed fix from #2 would require that every computed entity reference field item list to know that it needs to use a different trait than the standard one.

I think this should be better and actually fix all existing computed reference fields without the need for any changes on their part.

diff --git a/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
index 0a53d98..3fa199b 100644
--- a/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
+++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
@@ -24,7 +24,7 @@ public function getConstraints() {
    * {@inheritdoc}
    */
   public function referencedEntities() {
-    if (empty($this->list)) {
+    if ($this->isEmpty()) {
       return [];
     }

It would also mean that we don't need any change record update :)

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Title: referencedEntities() doesn't work for computed fields » EntityReferenceFieldItemList::referencedEntities() doesn't work for computed fields

Status: Needs review » Needs work

The last submitted patch, 5: entityreference-computed-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Status: Needs work » Needs review
FileSize
2978848-9.interdiff.txt2.13 KB
2978848-9.patch4.52 KB

@amateescu, yes that was my first thought but I missed that ->isEmpty() is aware of computed fields so I thought that it would make the method too complex :)

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Issue summary: View changes
Issue tags: -Needs change record updates

Untagged, updated IS.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Issue summary: View changes
amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Cool, looks good to me then :)

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented 
+++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
@@ -24,7 +24,7 @@ public function getConstraints() {
+    if ($this->isEmpty()) {

This is an elegant fix, nice!

I also did a quick scan for other instances of empty($this->list) which could be similarly problematic, but the only occurrence in CommentFieldItemList doesn't look like it would be appropriate for use with the trait/computed values. +1 RTBC.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Issue summary: View changes
alexpott’s picture

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

Committed and pushed 7896891da5 to 8.6.x and 7dfb2637ff to 8.5.x. Thanks!

  • alexpott committed 7896891 on 8.6.x 
    Issue #2978848 by claudiu.cristea, amateescu:...

  • alexpott committed 7dfb263 on 8.5.x 
    Issue #2978848 by claudiu.cristea, amateescu:...

Status: Fixed » Closed (fixed)

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

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +DER issue