There are many places in EntityReferenceFormatterTest where the code uses ->name->value when it should ideally use ->label().

This was identified by @larowlan while reviewing #2940605: Can only intentionally re-render an entity with references 20 times.

CommentFileSizeAuthor
#12 after-patch.png1.04 MBsourabhjain
#7 EntityReferenceFormatterTest-label-3299024-7.patch2.08 KBreenaraghavan
#4 3299024-3-EntityReferenceFormatterTest-label.patch1.86 KBamarlata

Issue fork drupal-3299024

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathanshaw created an issue. See original summary.

amarlata made their first commit to this issue’s fork.

amarlata opened merge request !2533

amarlata’s picture

amarlata
Amarlata
Primary language English
commented
StatusFileSize
new 3299024-3-EntityReferenceFormatterTest-label.patch1.86 KB

#patch created

amarlata’s picture

amarlata
Amarlata
Primary language English
commented
Status: Active » Needs review
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs review » Needs work

The patch changes look ok, but it's malformed in some way.

Easiest fix for a small change like this is to use the web IDE in the merge request. No need to make a patch file then.

reenaraghavan’s picture

reenaraghavan commented
Status: Needs work » Needs review
StatusFileSize
new EntityReferenceFormatterTest-label-3299024-7.patch2.08 KB

Modified EntityReferenceFormatterTest file where the code uses ->name->value

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
Status: Needs review » Reviewed & tested by the community

The changes from #7 look great and tests are passing, so I'm going to mark this as "Reviewed & Tested by the Community".

reenaraghavan’s picture

reenaraghavan commented

I have committed and pushed the MR for the same.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

@lucassc, thank you for reviewing the patch! The patch may look good but is it changing all instances of ->name->value? Have any been missed by accident? In cases like this it is better to apply the patch locally and search for any other instances that need to be changed.

@reenaraghavan, the patch file is sufficient. There is no need to have an MR.

Just one thing to do, setting this back to NW for confirmation that there are no other occurrences to fix.

Thanks!

sourabhjain’s picture

sourabhjain
Primary language English
commented
Assigned: Unassigned » sourabhjain

I will check the #10 comment concern.

sourabhjain’s picture

sourabhjain
Primary language English
commented
Assigned: sourabhjain » Unassigned
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new after-patch.png1.04 MB

I have reviewed the patch #7 and its looks fine and no other instance in of ->name()->value in EntityReferenceFormatterTest file.
Attaching the screenshot

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented

@quietone, thanks for mentoring!

alexpott credited larowlan.

alexpott’s picture

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

Committed and pushed 94a4059666 to 10.1.x and 84d1183c3a to 10.0.x and 5839fb2cab to 9.5.x. Thanks!

Crediting @larowlan for coming up with the issue and @quietone for mentoring.

Backported to 9.5.x as this is a test only fix.

  • alexpott committed 94a4059 on 10.1.x 
    Issue #3299024 by amarlata, reenaraghavan, quietone, larowlan:...

  • alexpott committed 84d1183 on 10.0.x 
    Issue #3299024 by amarlata, reenaraghavan, quietone, larowlan:...

  • alexpott committed 5839fb2 on 9.5.x 
    Issue #3299024 by amarlata, reenaraghavan, quietone, larowlan:...

Status: Fixed » Closed (fixed)

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