Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables
File /core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php
Line 855: Unused local variable $arg
Line 1470: Unused local variable $arg
Merged with:
#2072597-17: Remove Unused local variables from tests in the Views module (this issue, comment #17)
File /core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
Line 173: Unused local variable $ids
#2072597-17: Remove Unused local variables from tests in the Views module (this issue, comment #17)
File /core/modules/views/lib/Drupal/views/Tests/Plugin/StyleTableUnitTest.php
Line 132: Unused local variable $style_plugin
#2072601: Remove Unused local variable $count from /core/modules/views/lib/Drupal/views/Plugin/views/area/Result.php
File /core/modules/views/lib/Drupal/views/Plugin/views/area/Result.php
Line 71: Unused local variable $count
Line 75: Unused local variable $label
Line 77: Unused local variable $page_count
Line 82: Unused local variable $page_count
Line 90: Unused local variable $current_record_count
#2072613: Remove Unused local variable $view from /core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.php
File /core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.php
Line 222: Unused local variable $view
#2072617: Remove Unused local variable $base from /core/modules/views/lib/Drupal/views/Tests/Handler/FieldWebTest.php
File /core/modules/views/lib/Drupal/views/Tests/Handler/FieldWebTest.php
Line 207: Unused local variable $base
Line 208: Unused local variable $absolute_string
Line 232: Unused local variable $expected_result
Line 234: Unused local variable $result
Beta phase evaluation
Issue priority | Normal because nothing is broken. |
---|---|
Unfrozen changes | Unfrozen because this issue improves automated tests, performance, and DX. |
Comment | File | Size | Author |
---|---|---|---|
#88 | remove-unused-2072597-88.patch | 2.48 KB | tkuldeep17 |
#85 | remove-unused-2072597-85.patch | 2.35 KB | jgeryk |
#80 | remove-unused-2072597-80.patch | 1.95 KB | anish.a |
#78 | remove-unused-2072597-78.patch | 2.7 KB | Anonymous (not verified) |
#68 | remove-unused-2072597-68.patch | 3.47 KB | er.pushpinderrana |
Comments
Comment #1
legolasboRemoved unused local variables
Comment #2
phiit CreditAttribution: phiit commentedReviewed, patch fixes both $arg cases but after applying the patch, second $arg removal makes $handler unused variable in line 1473.
The $handler is not used in this foreach loop at all.
Comment #3
legolasboI'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 was actually used or not I found out that removing the code chunk doesn't break any tests while the result of the function changes. I think this code needs tests, but perhaps it is better to check for and add missing tests for the entire class in a follow up issue.
Comment #4
xjmI'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.
Comment #5
stuti.manandhar CreditAttribution: stuti.manandhar commentedAll mentioned four patches are merged into single one.
Comment #6
enhdless CreditAttribution: enhdless commentedPatch #5 failed to apply.
Comment #7
enhdless CreditAttribution: enhdless commentedComment #8
legolasboI was also working on this last night. attached patch is my attempt at a merge of the issues
Comment #9
legolasboComment #12
legolasbo8: core-views-remove-unused-vars-2072597-8.patch queued for re-testing.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch had whitespace so re-rolling without it.
Comment #14
parthipanramesh CreditAttribution: parthipanramesh commentedSorry but this apply doesn't apply properly..
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedJust tested on latest HEAD using git apply -v and applies cleanly - also passes tests - could you try again? If still issues please explain what the issue is so I can address it directly thanks as all seems OK here.
Comment #16
areke CreditAttribution: areke commentedThe 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.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedOK, first I went through all the 'known unused' issues for 'unused' local variables for views module from the meta issue list #2002650: [meta, no patch] improve maintainability by removing unused local variables:
#2072601: Remove Unused local variable $count from /core/modules/views/lib/Drupal/views/Plugin/views/area/Result.php done in #2072597: Remove Unused local variables from tests in the Views module
#2072613: Remove Unused local variable $view from /core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.php done in #2072597: Remove Unused local variables from tests in the Views module
#2072617: Remove Unused local variable $base from /core/modules/views/lib/Drupal/views/Tests/Handler/FieldWebTest.php done in #2072597: Remove Unused local variables from tests in the Views module
#2072625: Remove Unused local variable $view from /core/modules/views/lib/Drupal/views/Tests/Handler/FilterDateTest.php committed 4/12
#2072609: Remove Unused local variable $id from /core/modules/views/lib/Drupal/views/Tests/ModuleTest.php committed 3/12
#2067529: Remove Unused local variable $alias from /core/modules/views/lib/Drupal/views/ManyToOneHelper.php committed 4/12
#2072593: Remove Unused local variable $id from /core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php committed 4/12
#2060725: Remove Unused local variable $plugin from /core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php closed, unused variable already been removed
#2064289: Remove Unused local variables from /core/modules/views/views.theme.inc committed 14/8
#2065115: Remove Unused local variable $langcode from /core/modules/views/views.tokens.inc committed 2/9
#2065125: Remove Unused local variable $display_id from /core/modules/views/lib/Drupal/views/DisplayBag.php committed 8/15
#2067541: Remove Unused local variable $id from /core/modules/views/lib/Drupal/views/ViewStorageController.php committed 8/18
#2072583: Remove Unused local variable $converted from /core/modules/views/lib/Drupal/views/ViewExecutable.php committed 8/29
#2072587: Remove Unused local variable $filter_id from /core/modules/views/lib/Drupal/views/Plugin/views/exposed_form/InputRequired.php committed 8/29
#2072589: Remove Unused local variable $extra from /core/modules/views/lib/Drupal/views/Plugin/views/join/Subquery.php committed 8/29
#2072591: Remove Unused local variable $extra from /core/modules/views/lib/Drupal/views/Plugin/views/join/JoinPluginBase.php committed 8/29
#2072595: Remove Unused local variable $base_field from /core/modules/views/lib/Drupal/views/Plugin/views/relationship/GroupwiseMax.php committed 2/10
#2072599: Remove Unused local variable $placeholder from /core/modules/views/lib/Drupal/views/Plugin/views/filter/String.php committed 8/29
#2072603: Remove Unused local variable $key from /core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php committed 4/9
#2072607: Remove Unused local variable $fake_item from /core/modules/views/lib/Drupal/views/Plugin/views/style/StylePluginBase.php committed 8/29
#2072611: Remove Unused local variable $result_nodes_per_char from /core/modules/views/lib/Drupal/views/Tests/GlossaryTest.php committed 2/10
#2072615: Remove Unused local variable $edit from /core/modules/views/lib/Drupal/views/Tests/Plugin/PagerTest.php committed 2/10
#2072619: Remove Unused local variable $key from /core/modules/views/lib/Drupal/views/Tests/Handler/RelationshipTest.php committed 2/10
#2072621: Remove Unused local variable $string from /core/modules/views/lib/Drupal/views/Tests/Handler/ArgumentNullTest.php committed 2/10
#2072623: Remove Unused local variable $empty_text from /core/modules/views/lib/Drupal/views/Tests/Handler/FieldUnitTest.php closed, both variables used
#2067547: Remove Unused local variable $display_id from /core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php committed 2/10
#2057157: Remove Unused local variable $plugin from /core/modules/views/views.module committed 13/8
#2080023: Remove Unused local variable $tid from /core/modules/views/includes/ajax.inc closed, no longer relevant
#2081215: Remove Unused local variable $arg from /core/modules/views/lib/Drupal/views/Plugin/views/area/TokenizeAreaPluginBase.php committed 4/9
Then I applied patch from this issue and ran code inspector and went through each of the unused local variable instances it found. Here's the list in order of as it appears in PhpStorm Code Inspector, along with my notes for each:
So the attached patch removes 2 further unused local variables, with all the other occurrences accounted for.
On one of the relevant issues there is a currently unused test, instead of removing I found the relevant issues and updated them to inform to uncomment the test when they've been attended to, also I added some comments in this patch to link to those issues.
I've also fixed a couple of code style issues and updated the issue summary.
I saw the comment re the change from
views_get_view('test_view');
to\Drupal\views\Views::getView('test_view');
- although these are an easy change I haven't added any further to this issue, there are 100+ occurrences so I went searching for a META issue but couldn't find one - is there one for this issue?Anyway that should be all the unused local variables for views counted for.
Comment #18
areke CreditAttribution: areke commentedOk. I reviewed this again and it looks good, so +1 from me.
This probably does need some tests though.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commented"This probably does need some tests though." - if someone could elaborate I could try to write some tests, unsure where to start...
Comment #20
legolasboFile
/core/modules/views/lib/Drupal/views/Plugin/views/area/Result.php
contains several code chunks like the one below, where it loops trough some variables that were incorrectly marked as unused. Removing those variables doesn't break any tests, which implies there are no tests covering those variables. That might be a good place to start, I'm sure you'll find more code without tests while working on this.
Comment #21
jvranckx CreditAttribution: jvranckx commentedComment #22
jvranckx CreditAttribution: jvranckx commentedThe patch (comment #17) couldn't find the correct linenumbers. Fixed the patch so it applies correctly again.
Comment #23
jvranckx CreditAttribution: jvranckx commentedIssue still needs writing of tests to cover all unused variables.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedlooking at this as part of the Toronto sprint: just adding some knowledge -
it appears that the tool to detect unused variables came up with false positives. These variables were removed (should not have been!) and no tests failed.
So we need some tests that will fail if this happens again (or the code needs to be rewritten to prevent the false positives).
comment #20 above https://drupal.org/comment/8358373#comment-8358373 illustrates the code construct that confused the tool. The array of $items end up producing references to variables.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedsome more knowledge: the "unused" variables are located in Result (Result.php) which is used to support the "Global: Result Summary" which can be placed in the Footer of a view.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedfigured it out - a test case is needed that will verify that a Result Summary field gets processed properly. I have created a new test case and a supporting test view. will attach a patch soon...
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedI am attaching a patch that contains 1 new test case that utilizes a result Summary in its footer. Note that this patch is a test case only. No changes to the core code are involved.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedconverted the patch file to be unix style eol's
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedfixed the patch file format - last line had no return
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedhide a broken patch file
Comment #36
damiankloip CreditAttribution: damiankloip commentedI'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 :)
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedUnderstood - it was a little confusing as the "other" plugin area tests are located in a different folder so we could not find the test. We looked in folder core/modules/views/lib/Drupal/views/Tests/Handler and did not find a related test. So we made one. The existing test is alone in core/modules/views/Tests/Drupal/views/Tests/Plugin/area.
Can someone clarify the difference between the 2 folders?
I have hidden the not required patch #33.
Comment #38
damiankloip CreditAttribution: damiankloip commentedThe tests inside the lib folder are web tests/DUTB classes. The tests in the tests folder are for phpunit.
Comment #39
jgSnell CreditAttribution: jgSnell commentedWorking on this during core mentoring
Comment #40
dawehnersetting back to needs work.
Comment #41
bogdan khrupa CreditAttribution: bogdan khrupa commentedThis is a re-roll.
Comment #42
XanoAdding tag.
Comment #43
Taran2LRemove trailing spaces.
Comment #44
bogdan khrupa CreditAttribution: bogdan khrupa commentedRemove spaces.
Comment #45
bogdan khrupa CreditAttribution: bogdan khrupa commentedSorry i'm forget interdiff file.
Comment #47
bogdan khrupa CreditAttribution: bogdan khrupa commented17: core-views-remove-unused-vars-2072597-17.patch queued for re-testing.
Comment #49
damiankloip CreditAttribution: damiankloip commentedSeems like an unrelated failure.
Comment #50
Xano44: 2072597-44.patch queued for re-testing.
Comment #51
XanoSetting the status back to what it was before the test failure.
Comment #52
connorwk CreditAttribution: connorwk commentedHere at the Austin sprint, going to review this.
Comment #53
connorwk CreditAttribution: connorwk commentedRerolled, 3-way auto-merge.
No conflicts.
Just uploading the patch to see what the bot says, reviewing it now.
Comment #54
connorwk CreditAttribution: connorwk commentedThe 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.
Comment #56
martin107 CreditAttribution: martin107 commentedRestored some variables definitions removed in #54.
FieldWebTest.php is now green locally
Comment #57
damiankloip CreditAttribution: damiankloip commentedI think we can go the other way and remove the variables that are unused that use the globals that were removed before.
Comment #58
damiankloip CreditAttribution: damiankloip commentedIMO, this is not a good change. Yes, they are unused technically but this impacts the readability of these bits of code considerably.
Comment #59
amitgoyal CreditAttribution: amitgoyal commentedPlease review changes as suggested in #58.
Comment #60
XanoIt 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.
Comment #61
damiankloip CreditAttribution: damiankloip commentedYes, 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.
Comment #62
XanoAgreed.
Comment #63
alexpottThere 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
inDrupal\views\Tests\ViewStorageTest::displayMethodTests
Comment #64
joshi.rohit100review the patch.
Comment #65
jhedstrom+1 for removing unused code. This however needs a reroll.
Comment #66
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedRerolled.
Comment #68
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedComment #69
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedComment #76
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch worked fine locally so queued for re-testing.
Comment #78
Anonymous (not verified) CreditAttribution: Anonymous commentedAsked on irc and berdir said:
stevepurkiss: there's a segfault in the installer. has to be something about your patch because it doesn't happen without, although I can't say what
stevepurkiss: oh, that change in PathPluginBase is wrong. $pos *is* used
So I've removed the first chunk of code which deals with the PathPluginBase, see if this works.
Comment #79
Mile23Hey, 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
Comment #80
anish.a CreditAttribution: anish.a as a volunteer commentedThe above patch was no longer valid. Created new patch with similar modifications.
Although I am not sure how it works with latest code.
Comment #81
borisson_Comment #82
Mile23Nice 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:
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.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:
So let's change that to say:
Comment #83
jgeryk CreditAttribution: jgeryk commentedComment #84
jgeryk CreditAttribution: jgeryk commentedComment #85
jgeryk CreditAttribution: jgeryk commentedUpdated documentation as proposed
Comment #86
dawehnerThese changes look alright for me! Why do we still have the 'Needs tests' tag? Doesn't really makes sense for me IMHO.
Comment #87
Mile23I think it had 'needs tests' from scope creep.
Still needs proper @returns documentation, like this:
Comment #88
tkuldeep17 CreditAttribution: tkuldeep17 at Axelerant commentedHi Mile23,
Uploading patch by modifying @return documentation.
Comment #89
steinmb CreditAttribution: steinmb as a volunteer commentedFind this issue a bit confusing though the patch looks OK.
Comment #92
legolasboThe tests failed across many non-views related tests, all because of some mysql related error. It is my guess testbot lied, so let's retest.
Comment #93
legolasboYep, lies. so back to RTBC
Comment #94
alexpottCommitted 1c9d106 and pushed to 8.0.x. Thanks!