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

File /core/modules/views/lib/Drupal/views/ManyToOneHelper.php

Line 223: Unused local variable $alias

CommentFileSizeAuthor
#10 views-2067529-10.patch1.47 KBmrded
#7 views-2067529-7.patch1.47 KBmrded
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jlindsey15’s picture

Looking at the code:

$alias = $this->handler->tableAliases[$value] = $this->addTable($join, $this->handler->view->many_to_one_aliases[$field][$value]);

          // and set table_alias to the first of these.
          if (empty($this->handler->tableAlias)) {
            $this->handler->tableAlias = $alias;
          }

it doesn't seem like $alias is unused, unless I'm being stupid.

rhm5000’s picture

@jlindsey15 Yes after looking at the code:

$alias = $this->handler->tableAliases[$value] = $this->addTable($join, $this->handler->view->many_to_one_aliases[$field][$value]);

          // and set table_alias to the first of these.
          if (empty($this->handler->tableAlias)) {
            $this->handler->tableAlias = $alias;
          }

local variable $alias is not unused.

alexanderpas’s picture

It seems like $this->handler->tableAliases always gets one value on this line,

$alias = $this->handler->tableAliases[$value] = $this->addTable($join, $this->handler->view->many_to_one_aliases[$field][$value]);

causing the conditional if (empty($this->handler->tableAlias)) to always return FALSE.

as a result, the variable $alias seems to be unused.

In addition in the case $this->handler->tableAliases is empty, $alias must be empty too, $alias is equal to an item in $this->handler->tableAliases

rhm5000’s picture

@alexanderpas The conditional if (empty($this->handler->tableAlias)) is checking $this->handler->tableAlias not $this->handler->tableAliases, thus if (empty($this->handler->tableAlias)) can return True. I agree if $this->handler->tableAliases is empty, $alias must be empty, meaning $this->handler->tableAlias = $alias; could return empty causing the conditional if (empty($this->handler->tableAlias)) to be insufficient or wrong conditional.

joelpittet’s picture

mrded’s picture

Assigned: Unassigned » mrded
mrded’s picture

Status: Active » Needs review
FileSize
1.47 KB

I have also changed comment to drupal code standards.

thomas.fleming’s picture

Status: Needs review » Reviewed & tested by the community

The code style looks fine, and the patch applies.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/ManyToOneHelper.php
@@ -220,11 +220,11 @@ public function ensureMyTable() {
+          $this->handler->tableAliases[$value] = $this->addTable($join, $this->handler->view->many_to_one_aliases[$field][$value]);
+          // And set table_alias to the first of these.
           if (empty($this->handler->tableAlias)) {
-            $this->handler->tableAlias = $alias;
+            $this->handler->tableAlias = $this->handler->tableAliases[$value];
           }

Let's fix up the comment here too.. No need to start with "And" and we are not longer setting table_alias

mrded’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
joelpittet’s picture

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

Back to RTBC.

Xano’s picture

10: views-2067529-10.patch queued for re-testing.

Xano’s picture

10: views-2067529-10.patch queued for re-testing.

webchick’s picture

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.