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

File /core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php

Line 1031: Unused local variable $key
Line 1230: Unused local variable $joins
Line 1231: Unused local variable $fields
Line 1324: Unused local variable $group
Line 1327: Unused local variable $group
Line 1369: Unused local variable $external
Line 1385: Unused local variable $items
Line 1545: Unused local variable $index

CommentFileSizeAuthor
#9 core-remove-unused-vars-sql.php-2072603-9.patch2.58 KBlegolasbo
#3 core-remove-unused-vars-sql.php-2072603-3.patch2.82 KBlegolasbo
#1 core-remove-unused-vars-sql.php-2072603-1.patch2.58 KBlegolasbo

Comments

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Assigned: legolasbo » Unassigned
Status: Active » Needs review
StatusFileSize
new core-remove-unused-vars-sql.php-2072603-1.patch2.58 KB

Removed the unused variables

phiit’s picture

phiit commented
Status: Needs review » Needs work

Thanks for the patch!

Reviewed and this patch looks good, except I think that the declaration of $fields variable at the start of Sql class could be cleaned up? Since after the patch the use (or in this case the unuse) of the variable is removed from the code completely.

Wondering if this chunk could be cleaned up as well? Starting at row 77 after applying the patch:

  /**
   * An array of fields.
   */
  var $fields = array();
legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Status: Needs work » Needs review
StatusFileSize
new core-remove-unused-vars-sql.php-2072603-3.patch2.82 KB

Attached patch also removes code chunk mentioned above.

Status: Needs review » Needs work

The last submitted patch, core-remove-unused-vars-sql.php-2072603-3.patch, failed testing.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented

The removal of $fields causes exceptions in 3 tests. I'll look into these tests to see if they should also be cleaned up.

phiit’s picture

phiit commented

I was probably wrong about removing the $fields completely. I think the first patch you did will work fine in this case as it removes the unused variable issue here.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented

I agree you could be wrong about removing $fields completely, however I do think we should at least review the failing code to see how $fields is used there. Perhaps it is just legacy code doing stuff with an always empty array.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php
@@ -73,12 +73,6 @@ class Sql extends QueryPluginBase {
-

at least $this->fields is 100% used in the class.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Status: Needs work » Needs review
StatusFileSize
new core-remove-unused-vars-sql.php-2072603-9.patch2.58 KB

You are right dawehner, i feel kinda stupid for not looking for $this->fields.

I've attached a reroll of the original patch to be reviewed once again.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php
@@ -1227,9 +1227,6 @@ public function query($get_count = FALSE) {
 
-    $joins = $where = $having = $orderby = $groupby = '';
-    $fields = $distinct = array();

This change looks odd but these variables are indeed not needed.

webchick’s picture

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

Committed and pushed to 8.x. Thanks!

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