Remove Unused local variables from tests in the Views module

Reviewed, patch fixes both $arg cases but after applying the patch, second $arg removal makes $handler unused variable in line 1473.

@@ -1470,7 +1470,7 @@ public function getRenderTokens($item) {
       $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) {

The $handler is not used in this foreach loop at all.

I've re-scoped this issue to include unused local variables in the whole Views module. Please merge in the three patches from the other issues referencing this one, and also include any other unused local variables for that module.

Also, please file these as minor issues.

All mentioned four patches are merged into single one.

Patch #5 failed to apply.

Sorry but this apply doesn't apply properly..

The supplied patch does apply, and it takes care of the unused local variable issue in this file. Also, thank you for taking care of some other stuff in the patch like changing
views_get_view('test_view');
to
\Drupal\views\Views::getView('test_view');
However, it does look like there are some more unused local variables in the module that are not resolved by the latest patch (@xjm re-scoped the issue). These should also be taken care of.

Ok. I reviewed this again and it looks good, so +1 from me.
This probably does need some tests though.

The patch (comment #17) couldn't find the correct linenumbers. Fixed the patch so it applies correctly again.

Issue still needs writing of tests to cover all unused variables.

So we need some tests that will fail if this happens again (or the code needs to be rewritten to prevent the false positives).

I'm sorry, there is already test coverage for this class :/ Please see \Drupal\views\Tests\Plugin\area\ResultTest.

it looks like there is one unused variable that is actually unused in Drupal\views\Plugin\views\area\Result:

$count = count($this->view->result);

The patch in #17 was correct, the tests passed because that was indeed never needed :)

The tests inside the lib folder are web tests/DUTB classes. The tests in the tests folder are for phpunit.

Working on this during core mentoring

setting back to needs work.

This is a re-roll.

Adding tag.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php
@@ -1469,19 +1469,19 @@ public function getRenderTokens($item) {
+    ¶
...
+    ¶

Remove trailing spaces.

Remove spaces.

Sorry i'm forget interdiff file.

17: core-views-remove-unused-vars-2072597-17.patch queued for re-testing.

Seems like an unrelated failure.

44: 2072597-44.patch queued for re-testing.

Setting the status back to what it was before the test failure.

Here at the Austin sprint, going to review this.

Rerolled, 3-way auto-merge.
No conflicts.
Just uploading the patch to see what the bot says, reviewing it now.

The rerolled patch looks good.
I added to the patch by removing more unused variables and checking all other variables phpStorm states as unused.
Here is a list of what is done to fix and or why the other unused variables are fine as they are.
1. I fixed the StyleUnformattedTest.php which had two un-asserted variables making them unused.
2. There are four false positive variables in Result.php that are used but auto inserted into an array by a foreach statement tricking phpStorm into thinking they are unused.
3. There are unused variables in multiple files that were in the list function which can be left out as per the example here: http://us1.php.net/manual/en/function.list.php
4. In FieldWebTest.php there are two unused variables that are used but the line is commented out with a todo message above it so they can be ignored.
5. In FieldUnitTest.php there is a false positive of an unused variable. It gets used in an assert shortly after being created.
6. In JoinPluginBase.php there are two instances of the same variable that phpStorm says is unused although there is a statement a further down using it? Maybe its over written by a lower down statement and phpStorm realizes this? But otherwise they are used, the code just may need to be cleaned up.

Otherwise this just needs another review and it should be good.

Restored some variables definitions removed in #54.
FieldWebTest.php is now green locally

I think we can go the other way and remove the variables that are unused that use the globals that were removed before.

+++ b/core/modules/views/tests/src/Controller/ViewAjaxControllerTest.php
@@ -131,7 +131,7 @@ public function testAjaxView() {
+    list(, $executable) = $this->setupValidMocks();

@@ -167,7 +167,7 @@ public function testAjaxViewWithArguments() {
+    list(, $executable) = $this->setupValidMocks();

@@ -189,7 +189,7 @@ public function testAjaxViewWithPager() {
+    list(, $executable) = $this->setupValidMocks();

+++ b/core/modules/views/tests/src/EventSubscriber/RouteSubscriberTest.php
@@ -78,7 +78,7 @@ protected function setUp() {
+    list(, , $display_1, $display_2) = $this->setupMocks();

IMO, this is not a good change. Yes, they are unused technically but this impacts the readability of these bits of code considerably.

Please review changes as suggested in #58.

IMO, this is not a good change. Yes, they are unused technically but this impacts the readability of these bits of code considerably.

It can also give the impression these variables are used somewhere, which they aren't. I'm just saying that based on damiankloip's reviews here and mine in person at the Drupalcon Austin sprints, this seems to hinge on personal preference a bit.

As both solutions are valid PHP, and we don't have anything about this in our coding standards, I'd say either solution is good in this case.

Yes, this seems to totally come down to preference :) But hey - this is a views issue ;)

In general, this just outlines the terrible-ness of using the list() construct.

Agreed.

There are loads more unused variables in the views module - not just the list() examples - I agree with @damiankloip I prefer the readability of having the unused variables declared. For example $view in Drupal\views\Tests\ViewStorageTest::displayMethodTests

review the patch.

+1 for removing unused code. This however needs a reroll.

Rerolled.

Hey, congrats. Patch in #78 still applies. :-)

Added beta evaluation.

Still using list(), which I gather is OK, just not OK when you do this: list(,$second_value) as per #63.

Poking through, contrary to the issue summary, those variables are, in fact, unused in /core/modules/views/lib/Drupal/views/Plugin/views/area/Result.php

Can't verify that there aren't any other unused variables in Views until this drops: #2451435: Trait collision in Drupal\views\Plugin\views\field\Field

The above patch was no longer valid. Created new patch with similar modifications.

Although I am not sure how it works with latest code.

Nice work, thanks.

In this case it didn't seem to matter, but rerolling patches is a good skill: https://www.drupal.org/patch/reroll

This patch does two things:

1. It changes Drupal\views\Tests\Plugin\StyleUnformattedTest::testDefaultRowClasses() to assert that two counters are the same at the end of the test, which I think is probably a good idea.
2. It also changes the return signature of Drupal\Tests\views\Unit\EventSubscriber\RouteSubscriberTest::setupMocks() to return a 2-item array instead of a 4-item array.

The thing about that is, the documentation lists four items which will be returned:

   * @return array
   *   An array of Views mocks, including the executable, the view entity, and
   *   two display plugins.

So let's change that to say:

   * @return \Drupal\views\Plugin\views\display\DisplayRouterInterface[]|\PHPUnit_Framework_MockObject_MockObject[]
   *   An array of two mocked view displays.

Updated documentation as proposed

These changes look alright for me! Why do we still have the 'Needs tests' tag? Doesn't really makes sense for me IMHO.

I think it had 'needs tests' from scope creep.

+++ b/core/modules/views/tests/src/Unit/EventSubscriber/RouteSubscriberTest.php
@@ -140,8 +140,7 @@ public function testOnAlterRoutes() {
    *
    * @return array
-   *   An array of Views mocks, including the executable, the view entity, and
-   *   two display plugins.
+   *   An array of two mocked view displays
    */

Still needs proper @returns documentation, like this:

   * @return \Drupal\views\Plugin\views\display\DisplayRouterInterface[]|\PHPUnit_Framework_MockObject_MockObject[]
   *   An array of two mocked view displays.

Hi Mile23,

Uploading patch by modifying @return documentation.

Find this issue a bit confusing though the patch looks OK.

Committed 1c9d106 and pushed to 8.0.x. Thanks!