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

File /core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php

Line 204: Unused local variable $id
Line 353: Unused local variable $argument_id
Line 1011: Unused local variable $arg
Line 1109: Unused local variable $style
Line 1574: Unused local variable $type
Line 1689: Unused local variable $arg
Line 2080: Unused local variable $output

CommentFileSizeAuthor
#15 core-remove-unused-vars-displaypluginbase.php-2072593-15.patch3.9 KBdeneo
#9 core-remove-unused-vars-displaypluginbase.php-2072593-9.patch3.9 KBAnonymous (not verified)
#4 core-remove-unused-vars-displaypluginbase.php-2072593-4.patch4.13 KBAnonymous (not verified)
#4 2072593-3-4-interdiff.txt835 bytesAnonymous (not verified)
#3 core-remove-unused-vars-displaypluginbase.php-2072593-3.patch4.1 KBlegolasbo
#1 core-remove-unused-vars-displaypluginbase.php-2072593-1.patch3.39 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-displaypluginbase.php-2072593-1.patch3.39 KB

Removed unused variables

phiit’s picture

phiit commented
Status: Needs review » Needs work

Reviewed, patch clears all mentioned unused variables, but after removing the $arg at line 1023 (see below) $handler becomes unused variable.

@@ -1023,7 +1023,7 @@ public function getArgumentsTokens() {
       $tokens = $this->view->build_info['substitutions'];
     }
     $count = 0;
-    foreach ($this->view->display_handler->getHandlers('argument') as $arg => $handler) {
+    foreach ($this->view->display_handler->getHandlers('argument') as $handler) {

Remaining code that has unused $handler starts at line 1026 with the patch applied:

foreach ($this->view->display_handler->getHandlers('argument') as $handler) {
  $token = '%' . ++$count;
  if (!isset($tokens[$token])) {
    $tokens[$token] = '';
  }

  // Use strip tags as there should never be HTML in the path.
  // However, we need to preserve special characters like " that
  // were removed by check_plain().
  $tokens['!' . $count] = isset($this->view->args[$count - 1]) ? strip_tags(decode_entities($this->view->args[$count - 1])) : '';
}
legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Status: Needs work » Needs review
Issue tags: +Need tests
StatusFileSize
new core-remove-unused-vars-displaypluginbase.php-2072593-3.patch4.1 KB

I've had a look at the code chunk related to $handler and found a way to remove both $handler and $token. While figuring out if this code chunk is actually used i deleted it and ran almost all views tests. The tests didn't seem to break so i think this code needs tests. However i think we should check for and add missing tests for the entire class in a follow up issue.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 2072593-3-4-interdiff.txt835 bytes
new core-remove-unused-vars-displaypluginbase.php-2072593-4.patch4.13 KB

I have reviewed this patch, all looks OK except you are now using a function within a loop declaration, which may cause performance issues as it may perform on each loop iteration:

for ($count=1; $count <= count($this->view->display_handler->getHandlers('argument')); $count++) {

I've attached a patch and interdiff to take the calculation out of the loop declaration. Tried to choose an appropriate variable name but feel free to alter:

    $handlers = count($this->view->display_handler->getHandlers('argument'));
    for ($count=1; $count <= $handlers; $count++) {

Status: Needs review » Needs work
Issue tags: -Novice, -Need tests

The last submitted patch, core-remove-unused-vars-displaypluginbase.php-2072593-4.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
Issue tags: +Novice, +Need tests

#4: core-remove-unused-vars-displaypluginbase.php-2072593-4.patch queued for re-testing.

boran’s picture

boran commented
Component: other » views.module
Status: Needs review » Reviewed & tested by the community

Reviewed the patch + interdiff in #4
- all the changes make sense and are easy to read
- the patch applies cleanly
- views continues working

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new core-remove-unused-vars-displaypluginbase.php-2072593-9.patch3.9 KB

Patch re-rolled. Following section removed as no longer applicable:

@@ -1591,7 +1589,7 @@ public function buildOptionsForm(&$form, &$form_state) {
         $entities = entity_get_info();
         $entity_tables = array();
         $has_translation_handlers = FALSE;
-        foreach ($entities as $type => $entity_info) {
+        foreach ($entities as $entity_info) {
           $entity_tables[] = $entity_info['base_table'];
 
           if (!empty($entity_info['translation'])) {
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

D'oh, setting to "needs review".

mcrittenden’s picture

mcrittenden commented
Issue tags: -Needs reroll

Tags

johnmcc’s picture

johnmcc commented

Looks good and applies cleanly. +1.

areke’s picture

areke commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This still applies and works well.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -1022,17 +1022,17 @@ public function getArgumentsTokens() {
+    for ($count=1; $count <= $handlers; $count++) {

$count = 1 - missing spaces.

deneo’s picture

deneo commented
StatusFileSize
new core-remove-unused-vars-displaypluginbase.php-2072593-15.patch3.9 KB

fixed

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
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!

Status: Fixed » Closed (fixed)

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

oadaeh’s picture

oadaeh commented
Issue tags: -Novice, -Need tests +Needs tests

Looking only at the last patch, it looks like no tests were included. Is that true? It's also been a while, so it's possible they were added after this patch went in.