Problem/Motivation

The sniff misses the following case...

namespace Drupal\Tests\field\Kernel;
use Drupal\Core\Field\BaseFieldDefinition;
use Drupal\Tests\field\Kernel\FieldKernelTestBase;

/**
 * Tests the field widget manager.
 *
 * @group field
 */
class WidgetPluginManagerTest extends FieldKernelTestBase {

The use Drupal\Tests\field\Kernel\FieldKernelTestBase; is unnecessary because FieldKernelTestBase is in the same namespace as the class.

@dawehner discovered this whilst reviewing #2674408: Fix "Drupal.Classes.UnusedUseStatement" standard in core

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 2685877.9.patch4.55 KBalexpott
#8 2685877.8.patch5.07 KBalexpott
#8 4-8-interdiff.txt3.47 KBalexpott
#6 interdiff.txt1.32 KBdawehner
#4 2685877.4.patch4.77 KBalexpott
#3 2685877.3.patch1.4 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Related issues: +#2674408: Fix "Drupal.Classes.UnusedUseStatement" standard in core
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2685877.3.patch1.4 KB

Here's a failing test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2685877.4.patch4.77 KB

And here's the fix and also took the opportunity to remove dead code that confused me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I used this on core and found a few more unused uses... see #2674408: Fix "Drupal.Classes.UnusedUseStatement" standard in core

Also tests pass...

phpunit
PHPUnit 4.8.6 by Sebastian Bergmann and contributors.

.......................................................

Time: 6.54 seconds, Memory: 38.50Mb

OK (55 tests, 5 assertions)
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.32 KB 
+++ b/coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php
@@ -78,9 +70,34 @@ class Drupal_Sniffs_Classes_UnusedUseStatementSniff implements PHP_CodeSniffer_S
-        $classUsed      = $phpcsFile->findNext(T_STRING, ($classPtr + 1));
+        $classUsed = $phpcsFile->findNext(T_STRING, ($classPtr + 1));
...
+        $namespace_ptr = $phpcsFile->findPrevious([T_NAMESPACE], $stackPtr);

Any reason we don't use camelCase for this var as well?

I quickly tried another testcase, see interdiff, but this actually passes aready.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php
@@ -63,14 +63,6 @@ class Drupal_Sniffs_Classes_UnusedUseStatementSniff implements PHP_CodeSniffer_S
-        // Seek along the statement to get the last part, which is the
-        // class/interface name.
-        while (isset($tokens[($classPtr + 1)]) === true
-            && in_array($tokens[($classPtr + 1)]['code'], array(T_STRING, T_NS_SEPARATOR)) === true
-        ) {
-            $classPtr++;
-        }

    So this change is not related at all to the issue here? I'll make a separate commit for this. 

  2. +++ b/coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php
@@ -78,9 +70,34 @@ class Drupal_Sniffs_Classes_UnusedUseStatementSniff implements PHP_CodeSniffer_S
+        // Namespace check...

    Can we have a more elaborate comment what we are doing here? Like "Check if the referenced class is in the same namespace as the current file. If it is then the use statement is not necessary." 

  3. +++ b/coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php
@@ -78,9 +70,34 @@ class Drupal_Sniffs_Classes_UnusedUseStatementSniff implements PHP_CodeSniffer_S
+        $namespace_ptr = $phpcsFile->findPrevious([T_NAMESPACE], $stackPtr);

    This will probably break horribly when people use multiple namespace {} constructs and use statements are not at the top of the file. But I think we can ignore that edge case and assume such code is unlikely and solve this another time if needed. 

  4. +++ b/coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php
@@ -78,9 +70,34 @@ class Drupal_Sniffs_Classes_UnusedUseStatementSniff implements PHP_CodeSniffer_S
+            $namespace = trim($phpcsFile->getTokensAsString($namespace_ptr + 1, $nsEnd - $namespace_ptr - 1));

    This is a bit unreliable if there are odd spaces or comments between all the parts. I think we should do a loop instead that uses PHP_CodeSniffer_Tokens::$emptyTokens to ignore that. Collecting all namsespace parts in that loop. And then do the same for the use statement.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 4-8-interdiff.txt3.47 KB
new 2685877.8.patch5.07 KB

Thanks for the reviews
#6 - changed to camelCase

Re #7

  1. Well apart from the fact that it looks as though you should be able to use this to get the namespace of the use statement but it just does not work.
  2. Fixed
  3. I'm not sure it will break that horribly because we're using the find previous ... so we should find the namespace that encapsulates the use statement we're looking at - but yeah I agree with deferring this issue. But this issue is why I chose the find previous rather than just starting from the beginning. However the pre-existing code...
            // Only check use statements in the global scope.
        if (empty($tokens[$stackPtr]['conditions']) === false) {
            return;
        }

    Prevents this from being an issue an doing anything wrong.

  4. #7.4 I don't think this is unreliable... adding test for it.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2685877.9.patch4.55 KB

Hmmm #7.1 has already been committing to coder... rerolling.

  • klausi committed 1c8b5a4 on 8.x-2.x authored by alexpott 
    Issue #2685877 by alexpott, klausi: UnusedUseStatement sniff should...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Version: 8.x-2.6 » 8.x-2.x-dev
Status: Needs review » Fixed

Committed, thanks!

Fixed the coding standard errors during commit with

phpcs --standard=PHPCS coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php

and

phpcbf --standard=PHPCS coder_sniffer/Drupal/Sniffs/Classes/UnusedUseStatementSniff.php

Status: Fixed » Closed (fixed)

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