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

File /core/modules/search/lib/Drupal/search/SearchQuery.php

Line 463: Unused local variable $total

CommentFileSizeAuthor
#8 drupal-core_remove-unused-variable-2080551-8.patch770 bytesdeneo
#7 drupal-core_remove-unused-variable-2080551-7.patch769 bytesdeneo
#5 2080551-remove_unused_var_total.patch1.18 KBbeowulf1416
#3 2080551-remove_unused_variables-7828157.patch613 bytesbeowulf1416
#2 2080551-remove_unused_variables-7828156.patch609 bytesbeowulf1416

Comments

chertzog’s picture

chertzog
he/him
Primary language English
Location Harrisburg, PA, USA
commented
Status: Active » Needs review

As far as i know this $total is needed in this situation. So this would be a close works as designed.

      foreach ($this->multiply as $total) {
        $this->scoresArguments[':total_' . $i] = $sum;
        $i++;
      }
beowulf1416’s picture

beowulf1416 commented
StatusFileSize
new 2080551-remove_unused_variables-7828156.patch609 bytes

replaced foreach loop with for loop to remove unused local variable

beowulf1416’s picture

beowulf1416 commented
StatusFileSize
new 2080551-remove_unused_variables-7828157.patch613 bytes

fixed bug on patch

Status: Needs review » Needs work

The last submitted patch, 2080551-remove_unused_variables-7828157.patch, failed testing.

beowulf1416’s picture

beowulf1416 commented
Status: Needs work » Needs review
StatusFileSize
new 2080551-remove_unused_var_total.patch1.18 KB
areke’s picture

areke commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch doesn't apply anymore, so it should be re-rolled.

deneo’s picture

deneo commented
Status: Needs work » Needs review
StatusFileSize
new drupal-core_remove-unused-variable-2080551-7.patch769 bytes
deneo’s picture

deneo commented
StatusFileSize
new drupal-core_remove-unused-variable-2080551-8.patch770 bytes

The last submitted patch, 7: drupal-core_remove-unused-variable-2080551-7.patch, failed testing.

areke’s picture

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

Using a for loop is better in this case because of the fact that it removes the unused local variable. It also makes the code a little more readable. The latest patch applies cleanly and does what it's supposed to do so... RTBC.

areke’s picture

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

Using a for loop is better in this case because of the fact that it removes the unused local variable. It also makes the code a little more readable. The latest patch applies cleanly and does what it's supposed to do so... RTBC.

webchick’s picture

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

Very strange. Wonder why it was that way to begin with.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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