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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

init

nod_’s picture

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

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

FileSize
490.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

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • alexpott committed a34cb25 on 8.3.x
    Issue #2002650 by mrsinguyen, chertzog, legolasbo, Erik Erskine, rhm50,...

  • alexpott committed a34cb25 on 8.3.x
    Issue #2002650 by mrsinguyen, chertzog, legolasbo, Erik Erskine, rhm50,...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • alexpott committed a34cb25 on 8.4.x
    Issue #2002650 by mrsinguyen, chertzog, legolasbo, Erik Erskine, rhm50,...

  • alexpott committed a34cb25 on 8.4.x
    Issue #2002650 by mrsinguyen, chertzog, legolasbo, Erik Erskine, rhm50,...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

Status: Active » Fixed

In the intervening years things have moved on and we can automated this.

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core. A good place to for unused variables is #3106216: Remove unused variables from core.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

alexpott’s picture

Status: Fixed » Needs work

In discussion with xim, catch and larowlan, my earlier comment is incorrect. We should handle each unused variable on its own merit and do the work to work out why it is not used.

This can point to broken code or incomplete testing see #3157369: Use unused variable $filters from DateTimeSchemaTest for example. A useful tool for this is git log -S “SOME TEXT” which will search git commits for matching text to find out when the variable might have become unused. Without doing the work to show why the variable is unused the patch will not be committed. Also git blame can be useful as well.

So let's leave this open to track the downstream work. Note it is still important to automate this so we can prevent future regressions and catch this prior to the fact rather than after.

Sam152’s picture

I just NW'd 3 issues based on what I understand from #54:

#3164496: Unused variable $langcode in content_moderation module, ModerationLocaleTest.php
#3164498: Unused variable $entity_type_ids in content_moderation module, content_moderation.module
#3164500: Unused variable $id in content_moderation module, Permissions.php

I understand less variables to parse is easier to read, but the benefit is seriously marginal in situations like the following:

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php
@@ -342,7 +342,7 @@ public function testLanguageIndependentContentModeration() {
-      foreach ($revision->getTranslationLanguages() as $langcode => $language) {
+      foreach ($revision->getTranslationLanguages() as $language) {

Is it more appropriate to just close issues like this, or once this becomes part of the official standard, will these kind of changes be required anyway?

longwave’s picture

Should we try and deal with all unused key variables in foreach loops in a single issue, and then handle all the other cases individually?

catch’s picture

I've been marking foreach loop changes as 'by design', since it's often more readable to specify what the key is whether it gets used or not.

jungle’s picture

+1 to #56, foreach should not be an exception, especially, if we are going to apply a phpcs sniff/rule for unused local variables stickly in the future.

jungle’s picture

Title: [meta] improve maintainability by removing unused local variables » [meta, no patch] improve maintainability by removing unused local variables
Version: 8.9.x-dev » 9.1.x-dev
Category: Bug report » Task
Status: Needs work » Active
Issue tags: -Novice, -Will cause commit conflicts
jungle’s picture

The rule/sniff is DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable, to identify unused variables, we can enable this rule and run composer run phpcs locally.

andypost’s picture

There's just 4 child issues left, so makes sense to add this rule to core and count remains

Also there's https://github.com/sirbrillig/phpcs-variable-analysis

Meantime the patch to try with core

longwave’s picture

> foreach should not be an exception, especially, if we are going to apply a phpcs sniff/rule for unused local variables stickly in the future.

We could modify/extend a sniff to ignore keys in foreach loops, if policy is that they are allowed to be unused?

jungle’s picture

Re #62, it depends on commiters' favor. Per #57, I think we can go for it :)

BTW, the current rule can not detect unused $value in foreach, for example:

foreach($list as $key => $value) {
  do_something_with_key($key);
  // $value is unused. DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable can not detect that $value is an unused variable.
}
catch’s picture

We could modify/extend a sniff to ignore keys in foreach loops, if policy is that they are allowed to be unused?

I think we probably need a coding standards issue to make it official, but my feeling is that unused variables in foreach is a style decision - as opposed to other unused variables which are always going to be either cruft or bugs.

jungle’s picture

> allowUnusedForeachVariables (bool, default true): if set to true, unused values from the key => value syntax in a foreach loop will never be marked as unused.

From https://github.com/sirbrillig/phpcs-variable-analysis which shared by @andypost in #61

andypost’s picture

FileSize
19.36 KB

Sniffer in #62 shows 260 usages https://www.drupal.org/pift-ci-job/1811507

Also attaching phpstorm results in "core" (147 places)

jungle’s picture

For unused variables in Tests, I think they could be removed in one go, it's unnecessary digging into the history. Can we do it in one seperate issue?

longwave’s picture

Disagree with #67, an unused variable in a test might be a refactor gone wrong that wasn't spotted at the time, and we are not testing something we think we are testing.

jungle’s picture

Good point, thanks @longwave!

andypost’s picture

Re #65 this library listed after upgrade of drupal/coder 8.3.10

core9$ COMPOSER_ROOT_VERSION=9.1.x-dev composer update
...
core9$ ~/.composer/vendor/bin/composer-lock-diff --no-links
+------------------------------+---------+---------+
| Production Changes           | From    | To      |
+------------------------------+---------+---------+
| laminas/laminas-diactoros    | 2.3.1   | 2.4.1   |
| symfony/console              | v4.4.12 | v4.4.13 |
| symfony/debug                | v4.4.12 | v4.4.13 |
| symfony/dependency-injection | v4.4.12 | v4.4.13 |
| symfony/error-handler        | v4.4.12 | v4.4.13 |
| symfony/event-dispatcher     | v4.4.12 | v4.4.13 |
| symfony/http-foundation      | v4.4.12 | v4.4.13 |
| symfony/mime                 | v5.1.4  | v5.1.5  |
| symfony/process              | v4.4.12 | v4.4.13 |
| symfony/routing              | v4.4.12 | v4.4.13 |
| symfony/serializer           | v4.4.12 | v4.4.13 |
| symfony/translation          | v4.4.12 | v4.4.13 |
| symfony/validator            | v4.4.12 | v4.4.13 |
| symfony/var-dumper           | v5.1.4  | v5.1.5  |
| symfony/yaml                 | v4.4.12 | v4.4.13 |
+------------------------------+---------+---------+

+------------------------------------+---------+---------+
| Dev Changes                        | From    | To      |
+------------------------------------+---------+---------+
| drupal/coder                       | 8.3.9   | 8.3.10  |
| symfony/browser-kit                | v4.4.12 | v4.4.13 |
| symfony/css-selector               | v4.4.12 | v4.4.13 |
| symfony/dom-crawler                | v4.4.12 | v4.4.13 |
| symfony/filesystem                 | v4.4.12 | v4.4.13 |
| symfony/finder                     | v4.4.12 | v4.4.13 |
| symfony/lock                       | v4.4.12 | v4.4.13 |
| symfony/phpunit-bridge             | v5.1.4  | v5.1.5  |
| sirbrillig/phpcs-variable-analysis | NEW     | v2.8.3  |
+------------------------------------+---------+---------+

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

Can this be marked as fixed now, given that we completed #3106216: Remove unused variables from core and also we have PHPStan in core?

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

quietone’s picture

Status: Active » Fixed
Related issues: +#3348079: Fix DrupalPractice.CodeAnalysis.VariableAnalysis

I agree with longwave that this can be closed now. Any remaining instances of an unused variable can be worked on as a child of the Meta issue that is coordinating the remaining work of implementing the DrupalPractice sniff. #3348079: Fix DrupalPractice.CodeAnalysis.VariableAnalysis

Congratulations to everyone for managing these changes!

Thanks!

Status: Fixed » Closed (fixed)

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