Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables

File /core/modules/link/lib/Drupal/link/Plugin/field/field_type/LinkItem.php

Line 112: Unused local variable $item

CommentFileSizeAuthor
#6 2081169-6.patch649 bytesrivimey
#3 2081169-3.patch1007 bytesareke
#1 drupal8.other_.2081169-1.patch653 bytessmira
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smira’s picture

smira CreditAttribution: smira commented
Status: Active » Needs review
FileSize
drupal8.other_.2081169-1.patch653 bytes
parthipanramesh’s picture

parthipanramesh CreditAttribution: parthipanramesh commented
Issue summary: View changes
Status: Needs review » Needs work

sorry, patch failed..

areke’s picture

areke CreditAttribution: areke commented
Status: Needs work » Needs review
FileSize
2081169-3.patch1007 bytes

Status: Needs review » Needs work

The last submitted patch, 3: 2081169-3.patch, failed testing.

areke’s picture

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

3: 2081169-3.patch queued for re-testing.

rivimey’s picture

rivimey
she/her
CreditAttribution: rivimey commented
FileSize
2081169-6.patch649 bytes

Reviewed the patch and it seems perfectly sane. Applied the patch to current 8.x HEAD and it applies ok with an offset of 3 lines. I have attched a reroll of the patch to make it current. Recommend RTBC

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Remove Unused local variable $item from /core/modules/link/lib/Drupal/link/Plugin/field/field_type/LinkItem.php » Remove unused local variables from the link module
Component: other » link.module
Priority: Normal » Minor

Let's also check the rest of the module and confirm that there are no other unused local variables.

rivimey’s picture

rivimey
she/her
CreditAttribution: rivimey commented
Status: Needs review » Reviewed & tested by the community

The rest of that file (LinkItem.php) looks fine w.r.t unused variables: there aren't many to not be used.

The only thing I could possibly question is in function isEmpty() at line 121, where it seems to be assumed that get('url') returns an object that can have getValue applied. Is there any point in checking get()'s return value before dereferencing?

 public function isEmpty() {
    $value = $this->get('url')->getValue();
    return $value === NULL || $value === '';
  }
enhdless’s picture

enhdless CreditAttribution: enhdless commented

Patch #3 works great!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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