Skip to content

Commit

Permalink
Only catch what should be caught
Browse files Browse the repository at this point in the history
This changes the exceptions being caught in various `catch` statements to more specific ones.

This means that errors which should always have been thrown, will now throw and only the potentially expected (and acceptable) exceptions will now be caught.

Note:
* For the `Namespaces::getDeclaredName()` method, the `catch` has not been changed (other than switching from the PHPCS native `RuntimeException` to the PHPCSUtils one).
    The reason for this is that the method is explicitly documented as returning `false` for non-existent tokens.
    While this behaviour is not in line with other methods in PHPCSUtils, changing the behaviour could be seen as a breaking change, so should be done in a major release.

Includes test for where the behaviour of the functions is now different.
  • Loading branch information
jrfnl committed May 20, 2024
1 parent 9850475 commit 1c9176d
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 9 deletions.
7 changes: 4 additions & 3 deletions PHPCSUtils/AbstractSniffs/AbstractArrayDeclarationSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@

namespace PHPCSUtils\AbstractSniffs;

use PHP_CodeSniffer\Exceptions\RuntimeException;
use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\Exceptions\LogicException;
use PHPCSUtils\Exceptions\UnexpectedTokenType;
use PHPCSUtils\Tokens\Collections;
use PHPCSUtils\Utils\Arrays;
use PHPCSUtils\Utils\Numbers;
Expand Down Expand Up @@ -178,7 +179,7 @@ final public function process(File $phpcsFile, $stackPtr)
{
try {
$this->arrayItems = PassedParameters::getParameters($phpcsFile, $stackPtr);
} catch (RuntimeException $e) {
} catch (UnexpectedTokenType $e) {
// Parse error, short list, real square open bracket or incorrectly tokenized short array token.
return;
}
Expand Down Expand Up @@ -245,7 +246,7 @@ public function processArray(File $phpcsFile)
foreach ($this->arrayItems as $itemNr => $arrayItem) {
try {
$arrowPtr = Arrays::getDoubleArrowPtr($phpcsFile, $arrayItem['start'], $arrayItem['end']);
} catch (RuntimeException $e) {
} catch (LogicException $e) {
// Parse error: empty array item. Ignore.
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion PHPCSUtils/Utils/Namespaces.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@

namespace PHPCSUtils\Utils;

use PHP_CodeSniffer\Exceptions\RuntimeException;
use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\BackCompat\BCFile;
use PHPCSUtils\Exceptions\OutOfBoundsStackPtr;
use PHPCSUtils\Exceptions\RuntimeException;
use PHPCSUtils\Exceptions\UnexpectedTokenType;
use PHPCSUtils\Internal\Cache;
use PHPCSUtils\Tokens\Collections;
Expand Down
3 changes: 1 addition & 2 deletions PHPCSUtils/Utils/UseStatements.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

namespace PHPCSUtils\Utils;

use PHP_CodeSniffer\Exceptions\RuntimeException;
use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\Exceptions\OutOfBoundsStackPtr;
Expand Down Expand Up @@ -379,7 +378,7 @@ public static function splitAndMergeImportUseStatement(File $phpcsFile, $stackPt
try {
$useStatements = self::splitImportUseStatement($phpcsFile, $stackPtr);
$previousUseStatements = self::mergeImportUseStatements($previousUseStatements, $useStatements);
} catch (RuntimeException $e) {
} catch (ValueError $e) {
// Not an import use statement.
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,41 @@ final class AbstractArrayDeclarationSniffTest extends PolyfilledTestCase
'processComma',
];

/**
* Test receiving an expected exception when an invalid token pointer is passed.
*
* @return void
*/
public function testNonExistentToken()
{
$this->expectException('PHPCSUtils\Exceptions\OutOfBoundsStackPtr');
$this->expectExceptionMessage(
'Argument #2 ($stackPtr) must be a stack pointer which exists in the $phpcsFile object, 100000 given'
);

$mockObj = $this->getMockedClassUnderTest();

$mockObj->expects($this->never())
->method('processOpenClose');

$mockObj->expects($this->never())
->method('processKey');

$mockObj->expects($this->never())
->method('processNoKey');

$mockObj->expects($this->never())
->method('processArrow');

$mockObj->expects($this->never())
->method('processValue');

$mockObj->expects($this->never())
->method('processComma');

$mockObj->process(self::$phpcsFile, 100000);
}

/**
* Test that the abstract sniff correctly bows out when presented with a token which is not an array.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
<?php

/* testClosureUse */
$closure = function() use($bar) {};

/* testUseNamePlainAliased */
use MyNamespace \ YourClass as ClassAlias;

Expand Down
48 changes: 45 additions & 3 deletions Tests/Utils/UseStatements/SplitAndMergeImportUseStatementTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

namespace PHPCSUtils\Tests\Utils\UseStatements;

use PHPCSUtils\TestUtils\UtilityMethodTestCase;
use PHPCSUtils\Tests\PolyfilledTestCase;
use PHPCSUtils\Utils\UseStatements;

/**
Expand All @@ -22,9 +22,38 @@
*
* @since 1.0.0
*/
final class SplitAndMergeImportUseStatementTest extends UtilityMethodTestCase
final class SplitAndMergeImportUseStatementTest extends PolyfilledTestCase
{

/**
* Test passing a non-existent token pointer.
*
* @return void
*/
public function testNonExistentToken()
{
$this->expectException('PHPCSUtils\Exceptions\OutOfBoundsStackPtr');
$this->expectExceptionMessage(
'Argument #2 ($stackPtr) must be a stack pointer which exists in the $phpcsFile object, 100000 given'
);

UseStatements::splitAndMergeImportUseStatement(self::$phpcsFile, 100000, []);
}

/**
* Test receiving an expected exception when a non-supported token is passed.
*
* @return void
*/
public function testInvalidTokenPassed()
{
$this->expectException('PHPCSUtils\Exceptions\UnexpectedTokenType');
$this->expectExceptionMessage('Argument #2 ($stackPtr) must be of type T_USE;');

// 0 = PHP open tag.
UseStatements::splitAndMergeImportUseStatement(self::$phpcsFile, 0, []);
}

/**
* Test correctly splitting and merging a import `use` statements.
*
Expand Down Expand Up @@ -53,6 +82,15 @@ public function testSplitAndMergeImportUseStatement($testMarker, $expected, $pre
public static function dataSplitAndMergeImportUseStatement()
{
$data = [
'closure-use' => [
'testMarker' => '/* testClosureUse */',
// Same as previous, which, as this is the first test case, is an empty statements array.
'expected' => [
'name' => [],
'function' => [],
'const' => [],
],
],
'name-plain' => [
'testMarker' => '/* testUseNamePlainAliased */',
'expected' => [
Expand Down Expand Up @@ -118,7 +156,11 @@ public static function dataSplitAndMergeImportUseStatement()
],
];

$previousUse = [];
$previousUse = [
'name' => [],
'function' => [],
'const' => [],
];
foreach ($data as $key => $value) {
$data[$key]['previousUse'] = $previousUse;
$previousUse = $value['expected'];
Expand Down

0 comments on commit 1c9176d

Please sign in to comment.