Problem/Motivation

In #3106216: Remove unused variables from core we introduced the sniff DrupalPractice.CodeAnalysis.VariableAnalysis to detect unused variables in Drupal Core.
To narrow the scope the decision was made to exclude all the test-classes.

This issue is created to make that sniff run without errors on all test-classes.

Steps to reproduce

Proposed resolution

- Remove the line <exclude-pattern>*/tests/*</exclude-pattern> from sniff DrupalPractice.CodeAnalysis.VariableAnalysis in core/phpcs.xml.dist
- Fix all and any errors

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 3251754-full-phpstan-do-not-commit.patch97.04 KBmallezie
#20 3251754-20-9.4.x.patch104.67 KBlongwave
#20 3251754-20-10.0.x.patch96.31 KBlongwave

Issue fork drupal-3251754

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spokje created an issue. See original summary.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Active » Postponed

Postponed on #3106216: Remove unused variables from core

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: [PP-1] Use sniff DrupalPractice.CodeAnalysis.VariableAnalysis on */tests/* » Use sniff DrupalPractice.CodeAnalysis.VariableAnalysis on */tests/*
Status: Postponed » Active

The parent issue is committed so this can be worked on now.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Assigned: Unassigned » Spokje

Spokje opened merge request !1633

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Active » Needs review

Thanks @tstoeckler for the On-The-Fly review :)

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Assigned: Spokje » Unassigned
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

A few more comments, mostly things that look like they can be removed entirely now.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at resc. commented

No problem, and thanks for the fixes, looking pretty close now 👍

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Needs review
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

The MR looks good.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Thanks @longwave and @daffie for their eagled-eyed reviews.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Needs review
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

All the code changes look good to me.
The file core/phpcs.xml.dist has the exclude-pattern removed.
The testbot is happy.
For me it is RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

RTBC +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

This needs to go on 10.0.x first.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Ok...

What's the reasoning behind that, and would that reasoning apply to all RTBC-ed Coding standards issues?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@Spokje everything has to go into 10.x first because otherwise there is a future Drupal without this fixed. And yes it applies to all coding standards patches currently rtbc.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Thanks for clearing that up.

I'm sure somebody else would love to keep RTBCed coding standards up to date with the ever moving core releases for months, I've done so for a few issues that were RTBC for 2 months and longer. I'm not keen on doing that again for 2+ months for the D10 version and then the needs-a-backport D9.4.x version.

If those changes are too big, too disruptive, no fun to look through, fine with me, but then there should be a ruling about how big/how many changes a MR/patch can have as a maximum, because this seems now like a lot of time that would be more usefully spend elsewhere.

_drops EUR 0.02 and all Coding standards issue_

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3251754-20-10.0.x.patch96.31 KB
3251754-20-9.4.x.patch104.67 KB

Reuploading patch for 9.4.x, rerolled for 10.0.x.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

NW following #3174402: Fix unused variable $unpublished in TrackerTest.php

Also not sure why PHPStan is failing on AssertHelperTraitTest, maybe a symptom of #3259355: Always do a full phpstan analysis on DrupalCI?

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
CreditAttribution: mallezie at iO commented
Status: Needs work » Needs review
FileSize
3251754-full-phpstan-do-not-commit.patch97.04 KB

Testing if phpstan causes the problem here.

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
CreditAttribution: mallezie at iO commented
Status: Needs review » Needs work

PHPstan gives same result on both partial and full scan.

In our phpstan config we have

excludePaths:
# Skip test fixtures.
- */tests/fixtures/*.php

This skips the AssertHelperTestClass class.

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
CreditAttribution: mallezie at iO commented
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

AssertHelperTraitTest and the fixture will anyway be removed with #3261266: Remove deprecated code from the testing framework (base classes, listeners, etc), so we may want to postpone this on that

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented 
+++ b/core/modules/tracker/tests/src/Functional/TrackerTest.php
@@ -388,7 +388,7 @@ public function testTrackerCronIndexing() {
-    $unpublished = $this->drupalCreateNode([
+    $this->drupalCreateNode([

This is no longer unused.

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.

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.

quietone credited samaraof@ciandt.com.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Parent issue: » #2571965: [meta] Fix PHP coding standards in core

Closed #3338899: Remove unused variables [$resource,$key] from ResourceTest as a duplicate, adding credit.

Adding parent.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Parent issue: #2571965: [meta] Fix PHP coding standards in core » #2002650: [meta, no patch] improve maintainability by removing unused local variables

Discussed this longwave and they suggested a different parent. I am changing the parent now. I will add a comment to the previous parent since this is using a sniff.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Parent issue: #2002650: [meta, no patch] improve maintainability by removing unused local variables » #3348079: Fix DrupalPractice.CodeAnalysis.VariableAnalysis

Change parent to the meta implementing this sniff.

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.