Repository: spaze/phpstan-disallowed-calls (php)

Issue:
`disallowedSuperglobals` rule will fail in PHPStan 1.6+bleeding edge
Seems like the `disallowedSuperglobals` feature (#105) will break in the upcoming PHPStan version 1.6 with bleeding edge rules (which this repository is using to test itself), and in the upcoming major version for everyone because it is using the `parent` attribute which will be `null`:

https://github.com/spaze/phpstan-disallowed-calls/blob/214b02ac3ffaedc10b55b91e2301e5a0a2144cc0/src/Usages/SuperglobalUsages.php#L54

The reason is [memory optimisation](https://phpstan.org/blog/preprocessing-ast-for-custom-rules).

There are a few options mentioned in the [article](https://phpstan.org/blog/preprocessing-ast-for-custom-rules) but they seem like an overkill here (the custom node visitor) or would prevent PHPStan memory savings (the config change).

I think that just a list of superglobal names (`_GET`, `_POST`, [etc.](https://www.php.net/manual/en/language.variables.superglobals.php)) in the rule class would do – although I like the current superglobal detection (a variable that's not defined in the current scope is treated as a superglobal).

The risk of PHP adding a new superglobal, and thus the code not detecting it because it would be missing in the list, is probably super low and would be a deal big enough to notice rather sooner than later.

Having a hardcoded list of superglobal variable names would break the test here:
https://github.com/spaze/phpstan-disallowed-calls/blob/045c407c2d1ea47900aa7bae4254db555de604d0/tests/Usages/SuperglobalUsagesTest.php#L69-L72
But I'm fine with that. Shouldn't break others though.

Pull request:
Superglobal variable detection now checks a list of superglobals
Change superglobal usage detection to a reusable variable detection, with hard-coded list of superglobals. The idea is that maybe one day somebody would like to detect some other kind of variables or variables in general, so this can be reused, while adding only a factory class like `DisallowedSuperglobalFactory` and a new service to `extension.neon`.

Fix #108

Patch:
diff --git a/extension.neon b/extension.neon
index 11c55e0..91222e3 100644
--- a/extension.neon
+++ b/extension.neon
@@ -74,7 +74,7 @@ services:
 	- Spaze\PHPStan\Rules\Disallowed\DisallowedNamespaceFactory
 	- Spaze\PHPStan\Rules\Disallowed\DisallowedNamespaceHelper
 	- Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalFactory
-	- Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalHelper
+	- Spaze\PHPStan\Rules\Disallowed\DisallowedVariableHelper
 	-
 		factory: Spaze\PHPStan\Rules\Disallowed\Usages\NamespaceUsages(forbiddenNamespaces: %disallowedNamespaces%)
 		tags:
@@ -128,6 +128,6 @@ services:
 		tags:
 			- phpstan.rules.rule
 	-
-		factory: Spaze\PHPStan\Rules\Disallowed\Usages\SuperglobalUsages(disallowedSuperglobals: %disallowedSuperglobals%)
+		factory: Spaze\PHPStan\Rules\Disallowed\Usages\VariableUsages(disallowedVariables: @Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalFactory::getDisallowedVariables(%disallowedSuperglobals%))
 		tags:
 			- phpstan.rules.rule
diff --git a/src/DisallowedSuperglobalFactory.php b/src/DisallowedSuperglobalFactory.php
index 83fb004..b5245d6 100644
--- a/src/DisallowedSuperglobalFactory.php
+++ b/src/DisallowedSuperglobalFactory.php
@@ -5,15 +5,31 @@ namespace Spaze\PHPStan\Rules\Disallowed;
 
 use PHPStan\ShouldNotHappenException;
 
-class DisallowedSuperglobalFactory
+class DisallowedSuperglobalFactory implements DisallowedVariableFactory
 {
 
+	/**
+	 * @see https://www.php.net/variables.superglobals
+	 */
+	private const SUPERGLOBALS = [
+		'$GLOBALS',
+		'$_SERVER',
+		'$_GET',
+		'$_POST',
+		'$_FILES',
+		'$_COOKIE',
+		'$_SESSION',
+		'$_REQUEST',
+		'$_ENV',
+	];
+
+
 	/**
 	 * @param array<array{superglobal?:string, message?:string, allowIn?:string[], errorIdentifier?:string}> $config
-	 * @return DisallowedSuperglobal[]
+	 * @return DisallowedVariable[]
 	 * @throws ShouldNotHappenException
 	 */
-	public function createFromConfig(array $config): array
+	public function getDisallowedVariables(array $config): array
 	{
 		$superglobals = [];
 		foreach ($config as $disallowedSuperglobal) {
@@ -21,13 +37,16 @@ class DisallowedSuperglobalFactory
 			if (!$superglobal) {
 				throw new ShouldNotHappenException("'superglobal' must be set in configuration items");
 			}
-			$disallowedSuperglobal = new DisallowedSuperglobal(
+			if (!in_array($superglobal, self::SUPERGLOBALS, true)) {
+				throw new ShouldNotHappenException("{$superglobal} is not a superglobal variable");
+			}
+			$disallowedSuperglobal = new DisallowedVariable(
 				$superglobal,
 				$disallowedSuperglobal['message'] ?? null,
 				$disallowedSuperglobal['allowIn'] ?? [],
 				$disallowedSuperglobal['errorIdentifier'] ?? ''
 			);
-			$superglobals[$disallowedSuperglobal->getSuperglobal()] = $disallowedSuperglobal;
+			$superglobals[$disallowedSuperglobal->getVariable()] = $disallowedSuperglobal;
 		}
 		return array_values($superglobals);
 	}
diff --git a/src/DisallowedSuperglobalHelper.php b/src/DisallowedSuperglobalHelper.php
deleted file mode 100644
index 02e64a2..0000000
--- a/src/DisallowedSuperglobalHelper.php
+++ /dev/null
@@ -1,59 +0,0 @@
-<?php
-declare(strict_types = 1);
-
-namespace Spaze\PHPStan\Rules\Disallowed;
-
-use PHPStan\Analyser\Scope;
-use PHPStan\Rules\RuleError;
-use PHPStan\Rules\RuleErrorBuilder;
-
-class DisallowedSuperglobalHelper
-{
-
-	/** @var IsAllowedFileHelper */
-	private $isAllowedFileHelper;
-
-
-	public function __construct(IsAllowedFileHelper $isAllowedFileHelper)
-	{
-		$this->isAllowedFileHelper = $isAllowedFileHelper;
-	}
-
-
-	private function isAllowedPath(Scope $scope, DisallowedSuperglobal $disallowedSuperglobal): bool
-	{
-		foreach ($disallowedSuperglobal->getAllowIn() as $allowedPath) {
-			if ($this->isAllowedFileHelper->matches($scope, $allowedPath)) {
-				return true;
-			}
-		}
-		return false;
-	}
-
-
-	/**
-	 * @param string $superglobal
-	 * @param Scope $scope
-	 * @param DisallowedSuperglobal[] $disallowedSuperglobals
-	 * @return RuleError[]
-	 */
-	public function getDisallowedMessage(string $superglobal, Scope $scope, array $disallowedSuperglobals): array
-	{
-		foreach ($disallowedSuperglobals as $disallowedSuperglobal) {
-			if ($disallowedSuperglobal->getSuperglobal() === $superglobal && !$this->isAllowedPath($scope, $disallowedSuperglobal)) {
-				return [
-					RuleErrorBuilder::message(sprintf(
-						'Using %s is forbidden, %s',
-						$disallowedSuperglobal->getSuperglobal(),
-						$disallowedSuperglobal->getMessage()
-					))
-						->identifier($disallowedSuperglobal->getErrorIdentifier())
-						->build(),
-				];
-			}
-		}
-
-		return [];
-	}
-
-}
diff --git a/src/DisallowedSuperglobal.php b/src/DisallowedVariable.php
similarity index 70%
rename from src/DisallowedSuperglobal.php
rename to src/DisallowedVariable.php
index aad5ee4..1df6a1b 100644
--- a/src/DisallowedSuperglobal.php
+++ b/src/DisallowedVariable.php
@@ -3,11 +3,11 @@ declare(strict_types = 1);
 
 namespace Spaze\PHPStan\Rules\Disallowed;
 
-class DisallowedSuperglobal
+class DisallowedVariable
 {
 
 	/** @var string */
-	private $superglobal;
+	private $variable;
 
 	/** @var string|null */
 	private $message;
@@ -20,23 +20,23 @@ class DisallowedSuperglobal
 
 
 	/**
-	 * @param string $superglobal
+	 * @param string $variable
 	 * @param string|null $message
 	 * @param string[] $allowIn
 	 * @param string $errorIdentifier
 	 */
-	public function __construct(string $superglobal, ?string $message, array $allowIn, string $errorIdentifier)
+	public function __construct(string $variable, ?string $message, array $allowIn, string $errorIdentifier)
 	{
-		$this->superglobal = $superglobal;
+		$this->variable = $variable;
 		$this->message = $message;
 		$this->allowIn = $allowIn;
 		$this->errorIdentifier = $errorIdentifier;
 	}
 
 
-	public function getSuperglobal(): string
+	public function getVariable(): string
 	{
-		return $this->superglobal;
+		return $this->variable;
 	}
 
 
diff --git a/src/DisallowedVariableFactory.php b/src/DisallowedVariableFactory.php
new file mode 100644
index 0000000..9fe8927
--- /dev/null
+++ b/src/DisallowedVariableFactory.php
@@ -0,0 +1,15 @@
+<?php
+declare(strict_types = 1);
+
+namespace Spaze\PHPStan\Rules\Disallowed;
+
+interface DisallowedVariableFactory
+{
+
+	/**
+	 * @param array<array{variable?:string, message?:string, allowIn?:string[], errorIdentifier?:string}> $config
+	 * @return DisallowedVariable[]
+	 */
+	public function getDisallowedVariables(array $config): array;
+
+}
diff --git a/src/DisallowedVariableHelper.php b/src/DisallowedVariableHelper.php
new file mode 100644
index 0000000..a94bbd7
--- /dev/null
+++ b/src/DisallowedVariableHelper.php
@@ -0,0 +1,59 @@
+<?php
+declare(strict_types = 1);
+
+namespace Spaze\PHPStan\Rules\Disallowed;
+
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\RuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+
+class DisallowedVariableHelper
+{
+
+	/** @var IsAllowedFileHelper */
+	private $isAllowedFileHelper;
+
+
+	public function __construct(IsAllowedFileHelper $isAllowedFileHelper)
+	{
+		$this->isAllowedFileHelper = $isAllowedFileHelper;
+	}
+
+
+	private function isAllowedPath(Scope $scope, DisallowedVariable $disallowedVariable): bool
+	{
+		foreach ($disallowedVariable->getAllowIn() as $allowedPath) {
+			if ($this->isAllowedFileHelper->matches($scope, $allowedPath)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+
+	/**
+	 * @param string $variable
+	 * @param Scope $scope
+	 * @param DisallowedVariable[] $disallowedVariables
+	 * @return RuleError[]
+	 */
+	public function getDisallowedMessage(string $variable, Scope $scope, array $disallowedVariables): array
+	{
+		foreach ($disallowedVariables as $disallowedVariable) {
+			if ($disallowedVariable->getVariable() === $variable && !$this->isAllowedPath($scope, $disallowedVariable)) {
+				return [
+					RuleErrorBuilder::message(sprintf(
+						'Using %s is forbidden, %s',
+						$disallowedVariable->getVariable(),
+						$disallowedVariable->getMessage()
+					))
+						->identifier($disallowedVariable->getErrorIdentifier())
+						->build(),
+				];
+			}
+		}
+
+		return [];
+	}
+
+}
diff --git a/src/Usages/SuperglobalUsages.php b/src/Usages/SuperglobalUsages.php
deleted file mode 100644
index 58027e6..0000000
--- a/src/Usages/SuperglobalUsages.php
+++ /dev/null
@@ -1,93 +0,0 @@
-<?php
-declare(strict_types = 1);
-
-namespace Spaze\PHPStan\Rules\Disallowed\Usages;
-
-use PhpParser\Node;
-use PhpParser\Node\Expr\Assign;
-use PhpParser\Node\Expr\Variable;
-use PHPStan\Analyser\Scope;
-use PHPStan\Rules\Rule;
-use PHPStan\Rules\RuleError;
-use PHPStan\ShouldNotHappenException;
-use Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobal;
-use Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalFactory;
-use Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalHelper;
-
-/**
- * Reports on global usage.
- *
- * @package Spaze\PHPStan\Rules\Disallowed
- * @implements Rule<Variable>
- */
-class SuperglobalUsages implements Rule
-{
-
-	/** @var DisallowedSuperglobalHelper */
-	private $disallowedHelper;
-
-	/** @var DisallowedSuperglobal[] */
-	private $disallowedSuperglobals;
-
-
-	/**
-	 * @param DisallowedSuperglobalHelper $disallowedSuperglobalHelper
-	 * @param DisallowedSuperglobalFactory $disallowedSuperglobalFactory
-	 * @param array<array{superglobal?:string, message?:string}> $disallowedSuperglobals
-	 * @throws ShouldNotHappenException
-	 */
-	public function __construct(DisallowedSuperglobalHelper $disallowedSuperglobalHelper, DisallowedSuperglobalFactory $disallowedSuperglobalFactory, array $disallowedSuperglobals)
-	{
-		$this->disallowedHelper = $disallowedSuperglobalHelper;
-		$this->disallowedSuperglobals = $disallowedSuperglobalFactory->createFromConfig($disallowedSuperglobals);
-	}
-
-
-	public function getNodeType(): string
-	{
-		return Variable::class;
-	}
-
-
-	private function isVariableBeingAssigned(Variable $variable): bool
-	{
-		$parentNode = $variable->getAttribute('parent');
-		if (!($parentNode instanceof Assign)) {
-			return false;
-		}
-
-		return $variable === $parentNode->var;
-	}
-
-
-	/**
-	 * @param Variable $node
-	 * @param Scope $scope
-	 * @return RuleError[]
-	 * @throws ShouldNotHappenException
-	 */
-	public function processNode(Node $node, Scope $scope): array
-	{
-		if (!($node instanceof Variable)) {
-			throw new ShouldNotHappenException(sprintf('$node should be %s but is %s', Variable::class, get_class($node)));
-		}
-
-		// If it's an assignment, allow it since it might define the variable in the current scope.
-		if ($this->isVariableBeingAssigned($node)) {
-			return [];
-		}
-
-		$variableName = $node->name;
-		if (!is_string($variableName)) {
-			return [];
-		}
-
-		$definedVariables = $scope->getDefinedVariables();
-		if (in_array($variableName, $definedVariables, true)) {
-			return [];
-		}
-
-		return $this->disallowedHelper->getDisallowedMessage('$' . $variableName, $scope, $this->disallowedSuperglobals);
-	}
-
-}
diff --git a/src/Usages/VariableUsages.php b/src/Usages/VariableUsages.php
new file mode 100644
index 0000000..dc94d39
--- /dev/null
+++ b/src/Usages/VariableUsages.php
@@ -0,0 +1,68 @@
+<?php
+declare(strict_types = 1);
+
+namespace Spaze\PHPStan\Rules\Disallowed\Usages;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\Variable;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleError;
+use PHPStan\ShouldNotHappenException;
+use Spaze\PHPStan\Rules\Disallowed\DisallowedVariable;
+use Spaze\PHPStan\Rules\Disallowed\DisallowedVariableHelper;
+
+/**
+ * Reports on a variable name usage.
+ *
+ * @package Spaze\PHPStan\Rules\Disallowed
+ * @implements Rule<Variable>
+ */
+class VariableUsages implements Rule
+{
+
+	/** @var DisallowedVariableHelper */
+	private $disallowedHelper;
+
+	/** @var DisallowedVariable[] */
+	private $disallowedVariables;
+
+
+	/**
+	 * @param DisallowedVariableHelper $disallowedVariableHelper
+	 * @param DisallowedVariable[] $disallowedVariables
+	 */
+	public function __construct(DisallowedVariableHelper $disallowedVariableHelper, array $disallowedVariables)
+	{
+		$this->disallowedHelper = $disallowedVariableHelper;
+		$this->disallowedVariables = $disallowedVariables;
+	}
+
+
+	public function getNodeType(): string
+	{
+		return Variable::class;
+	}
+
+
+	/**
+	 * @param Variable $node
+	 * @param Scope $scope
+	 * @return RuleError[]
+	 * @throws ShouldNotHappenException
+	 */
+	public function processNode(Node $node, Scope $scope): array
+	{
+		if (!($node instanceof Variable)) {
+			throw new ShouldNotHappenException(sprintf('$node should be %s but is %s', Variable::class, get_class($node)));
+		}
+
+		$variableName = $node->name;
+		if (!is_string($variableName)) {
+			return [];
+		}
+
+		return $this->disallowedHelper->getDisallowedMessage('$' . $variableName, $scope, $this->disallowedVariables);
+	}
+
+}


Test patch:
diff --git a/tests/DisallowedSuperglobalFactoryTest.php b/tests/DisallowedSuperglobalFactoryTest.php
new file mode 100644
index 0000000..cf71de2
--- /dev/null
+++ b/tests/DisallowedSuperglobalFactoryTest.php
@@ -0,0 +1,44 @@
+<?php
+declare(strict_types = 1);
+
+namespace Spaze\PHPStan\Rules\Disallowed;
+
+use Generator;
+use PHPStan\ShouldNotHappenException;
+use PHPStan\Testing\PHPStanTestCase;
+
+class DisallowedSuperglobalFactoryTest extends PHPStanTestCase
+{
+
+	/**
+	 * @dataProvider superglobalsProvider
+	 * @param string $superglobal
+	 * @param class-string|null $exceptionClass
+	 */
+	public function testNonSuperglobalInConfig(string $superglobal, ?string $exceptionClass)
+	{
+		if ($exceptionClass) {
+			$this->expectException($exceptionClass);
+			$this->expectExceptionMessage("{$superglobal} is not a superglobal variable");
+		} else {
+			$this->expectNotToPerformAssertions();
+		}
+		(new DisallowedSuperglobalFactory())->getDisallowedVariables([['superglobal' => $superglobal]]);
+	}
+
+
+	public function superglobalsProvider(): Generator
+	{
+		yield ['$GLOBALS', null, null];
+		yield ['$_SERVER', null, null];
+		yield ['$_GET', null, null];
+		yield ['$_POST', null, null];
+		yield ['$_FILES', null, null];
+		yield ['$_COOKIE', null, null];
+		yield ['$_SESSION', null, null];
+		yield ['$_REQUEST', null, null];
+		yield ['$_ENV', null, null];
+		yield ['$foo', ShouldNotHappenException::class, '$foo is not a superglobal variable'];
+	}
+
+}
diff --git a/tests/Usages/SuperglobalUsagesTest.php b/tests/Usages/SuperglobalUsagesTest.php
index ca125a6..bdf41f1 100644
--- a/tests/Usages/SuperglobalUsagesTest.php
+++ b/tests/Usages/SuperglobalUsagesTest.php
@@ -7,7 +7,7 @@ use PHPStan\File\FileHelper;
 use PHPStan\Rules\Rule;
 use PHPStan\Testing\RuleTestCase;
 use Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalFactory;
-use Spaze\PHPStan\Rules\Disallowed\DisallowedSuperglobalHelper;
+use Spaze\PHPStan\Rules\Disallowed\DisallowedVariableHelper;
 use Spaze\PHPStan\Rules\Disallowed\IsAllowedFileHelper;
 
 class SuperglobalUsagesTest extends RuleTestCase
@@ -15,10 +15,9 @@ class SuperglobalUsagesTest extends RuleTestCase
 
 	protected function getRule(): Rule
 	{
-		return new SuperglobalUsages(
-			new DisallowedSuperglobalHelper(new IsAllowedFileHelper(new FileHelper(__DIR__))),
-			new DisallowedSuperglobalFactory(),
-			[
+		return new VariableUsages(
+			new DisallowedVariableHelper(new IsAllowedFileHelper(new FileHelper(__DIR__))),
+			(new DisallowedSuperglobalFactory())->getDisallowedVariables([
 				[
 					'superglobal' => '$GLOBALS',
 					'message' => 'the cake is a lie',
@@ -35,15 +34,7 @@ class SuperglobalUsagesTest extends RuleTestCase
 						'../src/*-allow/*.*',
 					],
 				],
-				[
-					'superglobal' => '$TEST_GLOBAL_VARIABLE',
-					'message' => 'the cake is a lie',
-					'allowIn' => [
-						'../src/disallowed-allowed/*.php',
-						'../src/*-allow/*.*',
-					],
-				],
-			]
+			])
 		);
 	}
 
@@ -66,10 +57,6 @@ class SuperglobalUsagesTest extends RuleTestCase
 				'Using $_GET is forbidden, the cake is a lie',
 				12,
 			],
-			[
-				'Using $TEST_GLOBAL_VARIABLE is forbidden, the cake is a lie',
-				19,
-			],
 		]);
 		$this->analyse([__DIR__ . '/../src/disallowed-allow/superglobalUsages.php'], []);
 	}
diff --git a/tests/src/disallowed-allow/superglobalUsages.php b/tests/src/disallowed-allow/superglobalUsages.php
index 79aea1f..2cb1306 100644
--- a/tests/src/disallowed-allow/superglobalUsages.php
+++ b/tests/src/disallowed-allow/superglobalUsages.php
@@ -13,17 +13,8 @@ function useSuperglobals()
 }
 
 
-function useVariableNotInScope()
+function useNonGlobalVariable()
 {
-	// should not be allowed, since it's not defined in this scope
-	echo $TEST_GLOBAL_VARIABLE;
-}
-
-
-function useVariableInScope()
-{
-	$TEST_GLOBAL_VARIABLE = '1234';
-
-	// should be allowed, since it's defined in the current scope
-	echo $TEST_GLOBAL_VARIABLE;
+	$randomVar = 'foo';
+	echo $randomVar;
 }
diff --git a/tests/src/disallowed/superglobalUsages.php b/tests/src/disallowed/superglobalUsages.php
index 7b15dfe..1fe3f1a 100644
--- a/tests/src/disallowed/superglobalUsages.php
+++ b/tests/src/disallowed/superglobalUsages.php
@@ -13,17 +13,8 @@ function useSuperglobals()
 }
 
 
-function useVariableNotInScope()
+function useNonGlobalVariable()
 {
-	// should not be allowed, since it's not defined in this scope
-	echo $TEST_GLOBAL_VARIABLE;
-}
-
-
-function useVariableInScope()
-{
-	$TEST_GLOBAL_VARIABLE = '1234';
-
-	// should be allowed, since it's defined in the current scope
-	echo $TEST_GLOBAL_VARIABLE;
+	$randomVar = 'foo';
+	echo $randomVar;
 }