Part of meta-issue #2002650: [meta] 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

Files: 
CommentFileSizeAuthor
#9 core-remove-unused-vars-sql.php-2072603-9.patch2.58 KBlegolasbo
PASSED: [[SimpleTest]]: [MySQL] 58,488 pass(es).
[ View ]
#3 core-remove-unused-vars-sql.php-2072603-3.patch2.82 KBlegolasbo
FAILED: [[SimpleTest]]: [MySQL] 58,392 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
#1 core-remove-unused-vars-sql.php-2072603-1.patch2.58 KBlegolasbo
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]

Comments

Assigned:legolasbo» Unassigned
Status:Active» Needs review
StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]

Removed the unused variables

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();

Status:Needs work» Needs review
StatusFileSize
new2.82 KB
FAILED: [[SimpleTest]]: [MySQL] 58,392 pass(es), 0 fail(s), and 10 exception(s).
[ View ]

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.

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

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.

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.

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,488 pass(es).
[ View ]

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.

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.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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