Problem/Motivation

Proposed resolution

Remove the lines of code that declare the unused variables.

Is there a way to automatically detect? No. See #12-#14.

Remaining tasks

  • (done) Identify each file that has an unused variable.
  • (done) Make one issue per file.
  • Also, write down reproducable steps to identifying per file, each unused variable. Might require IDE.
  • (done) Create the issue and list the variables in that issue.
  • (done) Add the file to the table.
  • (done) Link the issue in the table.
  • Do a few that are active or needs work.
  • Review a few. See hints for reviewers.

Hints

Be careful of an include_once. It might use the variables that an IDE says is unused, but it is actually used. There are very few places in core that do that, but be careful of it.

(Small note be careful of list(). Most people can ignore this.)

Hints for reviewers

  1. Look at the patch with dreditor (http://dreditor.org).
  2. Apply the patch, then
    • If it does not apply:
      • If you have time, reroll it. (reroll instructions: http://drupal.org/patch/reroll) Interdiffs usually do not make sense with rerolls, but you can add some info like saying if it was an automatic merge, if there were conflicts, and if you found the issue/commit that caused it to not apply, mention that. If you do not have time to reroll it, mark the status needs work, and add the Needs reroll tag.
      • If the patch cannot be applied because it was already fixed by other patch (the line/lines to fix dissapeared), find the issue where that patch was posted and reference it. Then, you can mark it as "Closed (won't fix)" . Example: #2080539: Remove Unused local variable $langcode from /core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
    • If it did apply, recheck (by opening the file in an IDE like phpstorm) if any new unused variables are revealed by the removal of other unused variables.
  3. Look at the code near the var. If you think the unused var.. actually needed to be used, you may have spotted a flaw that needs to be fixed. (For example: #2002708-7: Remove unused local variables from core/includes/common.inc and #2067551-5: /core/lib/Drupal/Core/Routing/MatcherDumper.php will never roll back its transaction.)
  4. Summarize what you checked, how you checked them, and your findings (instead of just saying "looks good"). For example #2080367-2: Remove Unused local variable $storageFactory from core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php.
  5. Also see https://drupal.org/contributor-tasks/review for more info.

From #10 When making an issue, put the number of the new issue between the # and @ and it will make a great link. (the @ shows who it is assigned to, if it is assigned). Remove the create link when putting the issue number in.

Anyone can use the create link to make the issue and then edit this issue summary.

User interface changes

No ui changes.

API changes

No api changes.

Comments

YesCT’s picture

Issue summary:View changes

Updated issue summary.

YesCT’s picture

Issue summary:View changes

init

nod_’s picture

StatusFileSize
new208.96 KB

Maybe to give an idea, here is what phpstorm tells me: 626 unused variables in our code.

Attached the html output listing files and variables. The xml file has line number.

nod_’s picture

Issue summary:View changes

noting something.

amateescu’s picture

Doing an issue per file sounds a bit too much, I'd propose at most one issue per folder in core/ (includes, lib, modules/*, tests, etc..). I started with the one for core/includes/* and added it to the issue summary.

nod_’s picture

nod_’s picture

StatusFileSize
new358.99 KB

uploading the xml separately, so that ppl don't need to mess with an archive to see what's up.

( edit ) I would agree. We did one issue per file for the same thing in JS and it just takes forever and there are a lot more PHP files. When it's during a sprint it's a bit different though, that could work.

YesCT’s picture

@alexpott asked for these to be one issue per file.
Some of them might need more discussion that are part of settings includes or list().

These are good first issues for new contributors, so while it might seem like a lot of work the pay back will be great.

YesCT’s picture

Issue summary:View changes

Updated issue summary.

YesCT’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

Updated issue summary.

kerasai’s picture

Issue summary:View changes

2002732

kerasai’s picture

Issue summary:View changes

Updated issue summary.

nod_’s picture

StatusFileSize
new490.22 KB

Is there an issue for unused "use" uses? if not here is the list of the 936 of them.

tim.plunkett’s picture

Beware that list, it includes Annotation classes which many IDEs don't count as being used. Removing them will break everything.

alexpott’s picture

Title:[meta] improve performance by removing unused local variables» [meta] improve maintainability by removing unused local variables

Let's not over-egg the performance impact here... each unused variable / line of code will consume memory and cpu time but it's minuscule compared to other factors.

One is very important is that each line of code we have in the codebase needs to be maintained therefore removing cruft is a good thing. It's part of making Drupal a nice place to code... less thinking hmmm... that looks unused... but it is really?

alexpott’s picture

Issue summary:View changes

Update triggered

YesCT’s picture

I need to figure out a way to parse the xml, or get it in a different format. We should expand the table to list more files so the issues can be made.

nod_’s picture

StatusFileSize
new940 bytes
new264.17 KB

Updated export and php script to generate the table and link to create issues.

Remove all underscore from the files below and it should work.

nod_’s picture

Issue summary:View changes

moving the hints, as the table grows, people will not see them

YesCT’s picture

Issue summary:View changes

just adding the whole table. need to check if issues already in the summary are there and remove them.

YesCT’s picture

Issue summary:View changes

using [#@] instead of [#} to see who it is assigned to

tstoeckler’s picture

Issue summary:View changes

Updated issue summary. Added 2 new issues.

tstoeckler’s picture

Issue summary:View changes

Updated issue summary. Removed stale rows from the second table.

tstoeckler’s picture

Issue summary:View changes

Updated issue summary for #2056445

jan.stoeckler’s picture

Issue summary:View changes

Updated issue summary.

jan.stoeckler’s picture

Issue summary:View changes

Updated issue summary.

jan.stoeckler’s picture

Issue summary:View changes

modified the listing, specifically concerning /modules/testswarm/testswarm.pages.inc

jan.stoeckler’s picture

Issue summary:View changes

added reference to new issue #2057019

legolasbo’s picture

Issue summary:View changes

- Created an issue for the removal of variables in /core/modules/user/user.module
- Removed table row for /core/modules/user/user.admin.inc because that file has no unused variables any more.

legolasbo’s picture

Issue summary:View changes

created an issue for core/modules/block/block.module

legolasbo’s picture

Issue summary:View changes

Moved my issues to the issue table.

kerasai’s picture

@nod_ this is awesome, thanks.

All clear to start knocking some of these out?

kerasai’s picture

Issue summary:View changes

Fixed a copy paste error and added issue for /core/modules/views/views.module

tstoeckler’s picture

Issue summary:View changes

Updated issue summary.

legolasbo’s picture

Issue summary:View changes

Created some issues and altered the tables accordingly

legolasbo’s picture

Issue summary:View changes

Created /core/lib/Drupal/Core/Database/ related issues and altered the tables accordingly

manu4543’s picture

Issue summary:View changes

Updated issue summary to include the new created issue.

manu4543’s picture

Issue summary:View changes

Updating issue summary for adding issue https://drupal.org/node/2061387

manu4543’s picture

Issue summary:View changes

Updated issue summary for adding issue https://drupal.org/node/2061397

duozersk’s picture

Issue summary:View changes

Created one sub-issue

duozersk’s picture

Issue summary:View changes

Added new sub-issue

duozersk’s picture

Issue summary:View changes

Added 2 sub-issues

duozersk’s picture

Issue summary:View changes

Added 2 more sub-issues

duozersk’s picture

Issue summary:View changes

Combined 3 lines into one.

duozersk’s picture

Issue summary:View changes

Added 3 more sub-issues

duozersk’s picture

Issue summary:View changes

Added 4 sub-issues.

sergeypavlenko’s picture

Issue summary:View changes

Assign /core/lib/Drupal/Core/Routing/RouteProvider.php

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

jlindsey15’s picture

Issue summary:View changes

Added NodeSaveTest issue

jlindsey15’s picture

Issue summary:View changes

Removed "create" button for nodesavetest issue

jlindsey15’s picture

Issue summary:View changes

Added issue

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

sergeypavlenko’s picture

Issue summary:View changes

Updated issue summary.

jlindsey15’s picture

Issue summary:View changes

Added child issue

jlindsey15’s picture

Issue summary:View changes

Added child issue

jlindsey15’s picture

Issue summary:View changes

Added child issue.

jlindsey15’s picture

Issue summary:View changes

Added child issue.

legolasbo’s picture

Issue summary:View changes

Cleaned up the second table by moving active issues to the first one

legolasbo’s picture

Issue summary:View changes

Created some issues

legolasbo’s picture

Issue summary:View changes

Created some more issues

mrsinguyen’s picture

Issue summary:View changes

Added the #2079857

mrsinguyen’s picture

Issue summary:View changes

Added #2079863

mrsinguyen’s picture

Issue summary:View changes

Added #2079881

mrsinguyen’s picture

Issue summary:View changes

Added #2079887

mrsinguyen’s picture

Issue summary:View changes

Added #2079891

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

mrsinguyen’s picture

Issue summary:View changes

Updated issue summary.

webchick’s picture

I'm curious about something. Is it possible to write some kind of automated test that will catch these as they're introduced? One would assume PHPStorm is hooking into some kind of PHP Reflection library or similar, but not sure. I am just concerned that we're going through all of this effort that's going to eventually be eroded away without some means of people without PHPStorm ensuring the code base stays sane.

dawehner’s picture

There are some places in core which let's IDEs think that variables are unused even they are not (sadly I don't remember the example).

http://phpmd.org/ seems to be what you want to do.

alexpott’s picture

In an ideal world we would be running phpmd as part of testbot. But it is going to take a while to get there.

alexpott’s picture

Issue summary:View changes

Updated issue summary.

YesCT’s picture

Issue summary:View changes

added info about how to move on to other types of issues.

YesCT’s picture

Wondering how to find other issues to work on?

These issues are nice ones for people to learn the core process. (After doing a few, and reviewing a few, find other issues in other topics by looking for other novice issues: https://drupal.org/project/issues/search/drupal?status%5B%5D=Open&versio... or other metas. See irc factoid: metas? Which links to the methods meta #2016679: Expand Entity Type interfaces to provide methods, protect the properties and properties rename meta #2052421: [META] Rename Views properties to core standards Another option is to jump in #drupal-contribute and ask for someone to recommend an issue to work on, or attend core mentoring office hours to find issues matched to your interest and skills.)

(updated issue summary)

YesCT’s picture

Issue summary:View changes

added html ending tag

YesCT’s picture

Issue summary:View changes

updated remaining tasks.

andypost’s picture

We should find better way to work on this kind of patches.

From IRC:

tell andypost the unused variable patches are also creating a testbot backlog. I think it's fine to teach someone how to create a patch for the very first time with those patches, but after that I feel they'r creating more overhead than the value we get out of them. Is there any way to graduate contributors from the unused variables to a remotely useful patch once they've got their first commit?

YesCT’s picture

Some guidance for directing people to other issues has been added... maybe some more, like 3 of these a day please? Along with the "how to find other issues" would alleviate some of the testbot strain.

The bot is back to 0 queued now. Did @jthorson end up spinning up the extra 20 testbots to clear it?

Hm. If we use this in Prague on the Get Involved Day... we will run into the same issue. Even if we have 50 people only do one of these a piece. Well no matter what people work on actually. It's just that they could do one of these *and* another issue during the day. I'd better talk to @jthorson.

mrsinguyen’s picture

it's my wrong when create many issues in yesterday. I think we need combine many issue for one patch.

mrsinguyen’s picture

Issue summary:View changes

small grammar fix

YesCT’s picture

Issue summary:View changes

added more hints and examples for reviewers.

YesCT’s picture

Issue summary:View changes

uh, i can write html. fixing.

YesCT’s picture

Issue summary:View changes

more html fix

YesCT’s picture

Issue summary:View changes

added hint about checking if it applies and reroll.

YesCT’s picture

Issue summary:View changes

another hint to say what was checked.

grisendo’s picture

Issue summary:View changes

Updated issue summary.

grisendo’s picture

Issue summary:View changes

Updated issue summary.

grisendo’s picture

Issue summary:View changes

Changed link to the issue with tokens

vijaycs85’s picture

Issue summary:View changes
vijaycs85’s picture

Issue summary:View changes

Cleaning up sub-tasks.

vijaycs85’s picture

Cleaning up sub-tasks.

vijaycs85’s picture

Issue summary:View changes

More update to sub-tasks

webchick’s picture

As much as I love initiatives like this to help get new people involved in core, these issues are definitely taking their toll. Over 50 of the 158 RTBC issues coming back from Thanksgiving were these issues. So tonight I literally spent 4 5 hours (and counting) doing just about nothing else but reviewing/committing/needs working these. That's 4 5 hours I didn't spend reviewing major/critical issues that help push D8 further towards release. :( And I only get about one of these 4 5-hour uninterrupted chunks per week.

This work is very valuable; a few of these have managed to catch some real actual bugs. But is it possible maybe to start combine some of these patches into bigger, less granular ones (per-file is just a bit nuts) so that the needs review/needs work/needs review/RTBC/commit cycle is more justified?

xjm’s picture

Please change the structure of this so that patches are rolled at a minimum on a per-module basis. Per file is unnecessarily granular.

Also, if you have already done 2 or 3 of these tasks, please leave the remainder for others to complete.

xjm’s picture

Also, please file all the sub-tasks as minor so that they can be sorted from normal issues.

xjm’s picture

Issue summary:View changes

Moved the fixed rows down so we can see what issues are still listed.

xjm’s picture

Issue summary:View changes

I reopened a couple that had actual bugs associated with them and retitled them to reflect their actual scope. When you discover a bug in the process of rolling a patch to remove several from a module, it makes sense to spin off a normal, targeted bug report. Thanks everyone!

webchick’s picture

Tagging.

aspilicious’s picture

Issue summary:View changes
aspilicious’s picture

Issue summary:View changes
YesCT’s picture

Issue summary:View changes

updating a bit of what issue is doing what.

YesCT’s picture

Issue summary:View changes

updating a bit of what issue is doing what.

YesCT’s picture

Issue summary:View changes

updating what core/tests is covering

YesCT’s picture

Issue summary:View changes

updating what core/tests is covering

lokapujya’s picture

Would be great to get this reviewed before I need to reroll again: #2080343: Remove Unused local variables from system module

lokapujya’s picture

Would be great to get this reviewed before I need to reroll again: #2080343: Remove Unused local variables from system module

jmarkel’s picture

Status:Active» Closed (fixed)

Closing this issue since it appears that none of the included issues appear to be open. If there are any remianing that are indeed still open they can be addressed as one-offs on their own.

andypost’s picture

Status:Closed (fixed)» Active

At least 2 issues open
#2072597: Remove Unused local variables from tests in the Views module
#2081137: Remove unused local variables from /core/includes/bootstrap.inc

And I'm sure there should be more filed, and we need to run inspections again!

xjm’s picture

Issue summary:View changes

Thanks everyone for your work on these issues!

In general, I'd recommend the following for managing these issues:

  • If the change involves rewriting a line or two of code to remove the unneeded variable assignment, it makes sense to do it in its own patch.
  • If the change is only deleting a line that has an unused variable assignment, we should combine all of those in a single patch, since it can be reviewed in one sitting simply by reading the diff and looking up each changed function.

Removing the table of issues from the summary since doing one patch per file is not desirable. Also toning down some of the claims about performance in the issue summary since in most cases the performance impact is immeasurably small on a typical request.