Skip to content

Commit

Permalink
Files/FileName: only clean up the excluded files when needed
Browse files Browse the repository at this point in the history
Efficiency tweak. Cleaning the ruleset passed "excluded files" doesn't need to be done time and again every single time this sniff is called.

For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient.

For test runs, this may need to be done more often, but that can be handled by storing the previous value of `$excluded_files_strict_check` property and checking if it has changed before doing the validation.

This commit implements this.

Additionally it adds some extra safeguards for incorrectly passed "excluded files" paths.

Includes removing the `$phpcsFile` parameter from the `is_file_excluded()` method as it is no longer needed by that method.

Includes additional unit tests.
Includes updating a pre-existing test to pass duplicate excluded files in different ways.
  • Loading branch information
jrfnl committed Nov 19, 2023
1 parent ac6bec0 commit a0e1125
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 20 deletions.
104 changes: 85 additions & 19 deletions Yoast/Sniffs/Files/FileNameSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Util\Common;
use PHPCSUtils\Tokens\Collections;
use PHPCSUtils\Utils\ObjectDeclarations;
use PHPCSUtils\Utils\TextStrings;
Expand Down Expand Up @@ -89,6 +88,22 @@ final class FileNameSniff implements Sniff {
*/
private $clean_oo_prefixes = [];

/**
* Cache of previously set list of excluded files.
*
* Prevents having to do the same file validation over and over again.
*
* @var array<string>
*/
private $previous_excluded_files = [];

/**
* Validated & cleaned up list of absolute paths to the excluded files.
*
* @var array<string, int> Key is the path, value irrelevant.
*/
private $validated_excluded_files = [];

/**
* Returns an array of tokens this test wants to listen for.
*
Expand Down Expand Up @@ -248,30 +263,15 @@ public function process( File $phpcsFile, $stackPtr ) {
* @return bool
*/
private function is_file_excluded( File $phpcsFile, $path_to_file ) {
$exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check );

if ( empty( $exclude ) ) {
$this->validate_excluded_files( $phpcsFile );
if ( empty( $this->validated_excluded_files ) ) {
return false;
}

$exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude );
$path_to_file = $this->normalize_directory_separators( $path_to_file );

if ( ! isset( $phpcsFile->config->basepath ) ) {
$phpcsFile->addWarning(
'For the exclude property to work with relative file path files, the --basepath needs to be set.',
0,
'MissingBasePath'
);
}
else {
$base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath );
$path_to_file = Common::stripBasepath( $path_to_file, $base_path );
}

$path_to_file = \ltrim( $path_to_file, '/' );

return \in_array( $path_to_file, $exclude, true );
return isset( $this->validated_excluded_files[ $path_to_file ] );
}

/**
Expand Down Expand Up @@ -323,4 +323,70 @@ private function validate_oo_prefixes() {
\rsort( $this->clean_oo_prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) );
}
}

/**
* Validate the list of excluded files passed from a custom ruleset.
*
* This will only need to be done once in a normal PHPCS run, though for
* tests the function may be called multiple times.
*
* @param File $phpcsFile The file being scanned.
*
* @return void
*/
private function validate_excluded_files( $phpcsFile ) {
// The basepath check needs to be done first as otherwise the previous/current comparison would be broken.
if ( ! isset( $phpcsFile->config->basepath ) ) {
$phpcsFile->addWarning(
'For the exclude property to work with relative file path files, the --basepath needs to be set.',
0,
'MissingBasePath'
);

// Only relevant for the tests: make sure previously set validated paths are cleared out.
$this->validated_excluded_files = [];

// No use continuing as we can't turn relative paths into absolute paths.
return;
}

if ( $this->previous_excluded_files === $this->excluded_files_strict_check ) {
return;
}

// Set the cache *before* validation so as to not break the above compare.
$this->previous_excluded_files = $this->excluded_files_strict_check;

// Reset a potentially previous set validated value.
$this->validated_excluded_files = [];

$exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check );
if ( empty( $exclude ) ) {
return;
}

$base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath );
$exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude );

foreach ( $exclude as $relative ) {
if ( \strpos( $relative, '..' ) !== false ) {
// Ignore paths containing path walking.
continue;
}

if ( \strpos( $relative, './' ) === 0 ) {
$relative = \substr( $relative, 2 );
}

/*
* Note: no need to check if the file really exists. We'll be doing a literal absolute path comparison,
* so if the file doesn't exist, it will never match.
*/
$this->validated_excluded_files[] = $base_path . '/' . $relative;
}

if ( ! empty( $this->validated_excluded_files ) ) {
$this->validated_excluded_files = \array_flip( $this->validated_excluded_files );
}
}
}
3 changes: 3 additions & 0 deletions Yoast/Tests/Files/FileNameUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ final class FileNameUnitTest extends AbstractSniffUnitTest {
'excluded-CLASS-file.inc' => 1, // Lowercase expected.
'excluded-backslash-file.inc' => 0,
'excluded-class-wrong-case.inc' => 1, // Filename not in line with class name. File not excluded due to wrong case used.
'excluded-illegal.inc' => 1, // Filename not in line with class name. File not excluded due to illegal path setting.
'excluded-multiple.inc' => 0,
'excluded-dot-prefixed.inc' => 0,

// Interface file names.
'outline-something-interface.inc' => 0,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!-- Annotation must be on line 2 as this sniff throws issues on line 1 and PHPCS ignores errors on annotation lines. -->
phpcs:set Yoast.Files.FileName excluded_files_strict_check[] ./classes/excluded-dot-prefixed.inc

<?php

class Some_Class {}

// phpcs:set Yoast.Files.FileName excluded_files_strict_check[]
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!-- Annotation must be on line 2 as this sniff throws issues on line 1 and PHPCS ignores errors on annotation lines. -->
phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes/../classes/excluded-illegal.inc

<?php

class Some_Class {}

// phpcs:set Yoast.Files.FileName excluded_files_strict_check[]
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!-- Annotation must be on line 2 as this sniff throws issues on line 1 and PHPCS ignores errors on annotation lines. -->
phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes/excluded-CLASS-file.inc,classes/excluded-multiple.inc

<?php

class Some_Class {}

// phpcs:set Yoast.Files.FileName excluded_files_strict_check[]
2 changes: 1 addition & 1 deletion Yoast/Tests/Files/FileNameUnitTests/excluded-file.inc
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!-- Annotation must be on line 2 as this sniff throws issues on line 1 and PHPCS ignores errors on annotation lines. -->
phpcs:set Yoast.Files.FileName excluded_files_strict_check[] excluded-file.inc
phpcs:set Yoast.Files.FileName excluded_files_strict_check[] excluded-file.inc,./excluded-file.inc,/excluded-file.inc

<?php

Expand Down

0 comments on commit a0e1125

Please sign in to comment.