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

File /core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php

Line 271: Unused local variable $mlid

Line 534: function verifyForums includes a $node_user argument which is unused also.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrsinguyen’s picture

Status: Active » Needs review
FileSize
645 bytes
grisendo’s picture

The patch attached is not for this issue, is the corresponding to #2080539
I send to re-test because it also can't be applied via git.

I attach the real one.

grisendo’s picture

angel.h’s picture

Assigned: Unassigned » angel.h
angel.h’s picture

Status: Needs review » Needs work

The patch looks fine but I get 2 more unused variables in this file when running PHPMD:
core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php:528 Avoid unused parameters such as '$node_user'.
core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php:629 Avoid unused parameters such as '$forum'.
They are not such performance trouble but still should be included in this ticket.

angel.h’s picture

Assigned: angel.h » Unassigned
Status: Needs work » Needs review
FileSize
2.99 KB
3.44 KB

Here is the new patch and interdiff.

royal121’s picture

The latest patch applies cleanly. Thanks.

areke’s picture

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

+1 from me as well. This looks good... RTBC

webchick’s picture

Assigned: Unassigned » larowlan
Status: Reviewed & tested by the community » Needs review

Hm. That's quite a significant change. Sending to larowlan (Forum module maintainer) for review.

xjm’s picture

Title: Remove Unused local variable $mlid from /core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php » Remove unused local variables in the forum module
Priority: Normal » Minor

Let's also check the rest of the module for unused local variables.

parthipanramesh’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

xjm’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

The changes look fine to me to but @webchick has asked for @larowlan's input and the issue summary could do with an update.

larowlan’s picture

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

Yes, that parameter is unused as well.

larowlan’s picture

There are three unused use statements in forum.module - should they be removed here too?
these three:

use Drupal\Core\Language\Language;
use Drupal\taxonomy\Entity\Term;
use Drupal\node\NodeInterface;
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

As we're cleaning unused things - yep lets remove the unused use statements too :)

larowlan’s picture

please don't credit me for this

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Looking good!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Too late. :P

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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