diff --git a/README.md b/README.md index 1894b71..6be388d 100644 --- a/README.md +++ b/README.md @@ -8,33 +8,32 @@ Composer-installable PHPStan rules for OpenEMR core and module development. Enfo composer require --dev opencoreemr/openemr-phpstan-rules ``` -## Usage +The rules are automatically loaded via [phpstan/extension-installer](https://github.com/phpstan/extension-installer). No manual configuration needed. -### For OpenEMR Core Development +**Important:** Do not manually include `extension.neon` in your phpstan configuration. The extension-installer handles this automatically. Adding a manual include will cause "File included multiple times" warnings. -Include the core ruleset in your `phpstan.neon`: +## Bundled Extensions -```neon -includes: - - vendor/opencoreemr/openemr-phpstan-rules/core.neon -``` +This package includes and configures these PHPStan extensions: -### For OpenEMR Module Development +- **[spaze/phpstan-disallowed-calls](https://github.com/spaze/phpstan-disallowed-calls)** - Forbids legacy function calls +- **[phpstan/phpstan-deprecation-rules](https://github.com/phpstan/phpstan-deprecation-rules)** - Reports usage of deprecated code -Include the module ruleset in your `phpstan.neon`: +## Rules -```neon -includes: - - vendor/opencoreemr/openemr-phpstan-rules/module.neon -``` +### Why Custom Rules Instead of Just `@deprecated`? -## Rules +This package provides custom rules that forbid specific functions by name (e.g., `sqlQuery()`, `call_user_func()`). You might wonder why we don't just mark these functions as `@deprecated` in OpenEMR and rely on `phpstan-deprecation-rules`. + +**The reason: module analysis without OpenEMR loaded.** -### Core Rules (for OpenEMR Core and Modules) +When running PHPStan on a standalone OpenEMR module, OpenEMR core may not be installed as a dependency or autoloaded. PHPStan's deprecation rules require the actual function/class definitions to read `@deprecated` annotations. If OpenEMR isn't available at scan-time, those annotations can't be read. -#### Database Rules +Our custom rules work by **function name matching**, so they catch forbidden calls even when the function definitions aren't available. This ensures modules get the same static analysis protection whether they're analyzed standalone or within a full OpenEMR installation. -**ForbiddenFunctionsRule** +### Database Rules + +**Disallowed SQL Functions** (via spaze/phpstan-disallowed-calls) - **Forbids:** Legacy `sql.inc.php` functions (`sqlQuery`, `sqlStatement`, `sqlInsert`, etc.) - **Requires:** `QueryUtils` methods instead - **Example:** @@ -50,7 +49,7 @@ includes: - **Forbids:** Laminas-DB classes (`Laminas\Db\Adapter`, `Laminas\Db\Sql`, etc.) - **Requires:** `QueryUtils` or `DatabaseQueryTrait` -#### Globals Rules +### Globals Rules **ForbiddenGlobalsAccessRule** - **Forbids:** Direct `$GLOBALS` array access @@ -65,7 +64,7 @@ includes: $value = $globals->get('some_setting'); ``` -#### Testing Rules +### Testing Rules **NoCoversAnnotationRule** - **Forbids:** `@covers` annotations on test methods @@ -75,11 +74,48 @@ includes: - **Forbids:** `@covers` annotations on test classes - **Rationale:** Same as above - incomplete coverage tracking -### Module-Specific Rules (for OpenEMR Modules Only) +### HTTP Rules + +**ForbiddenCurlFunctionsRule** +- **Forbids:** Raw `curl_*` functions (`curl_init`, `curl_exec`, `curl_setopt`, etc.) +- **Requires:** PSR-18 HTTP client +- **Example:** + ```php + // ❌ Forbidden + $ch = curl_init($url); + curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + $response = curl_exec($ch); + + // ✅ Required - use a PSR-18 HTTP client + $response = $httpClient->sendRequest($request); + ``` + +### Legacy PHP Rules + +**Disallowed call_user_func** (via spaze/phpstan-disallowed-calls) +- **Forbids:** `call_user_func()` and `call_user_func_array()` +- **Requires:** First-class callables (PHP 8.1+) +- **Example:** + ```php + // ❌ Forbidden + call_user_func([$object, 'method'], $arg1, $arg2); + call_user_func_array('someFunction', $args); -These additional rules enforce Symfony-inspired MVC patterns in OpenEMR modules. + // ✅ Required - first-class callable syntax + $callable = $object->method(...); + $callable($arg1, $arg2); -#### CatchThrowableNotExceptionRule + $callable = someFunction(...); + $callable(...$args); + + // Static methods + $callable = SomeClass::staticMethod(...); + $callable($arg); + ``` + +### Exception Handling Rules + +**CatchThrowableNotExceptionRule** - **Forbids:** `catch (\Exception $e)` - **Requires:** `catch (\Throwable $e)` - **Rationale:** Catches both exceptions and errors (`TypeError`, `ParseError`, etc.) @@ -100,7 +136,9 @@ These additional rules enforce Symfony-inspired MVC patterns in OpenEMR modules. } ``` -#### NoSuperGlobalsInControllersRule +### Controller Rules + +**NoSuperGlobalsInControllersRule** - **Forbids:** `$_GET`, `$_POST`, `$_FILES`, `$_SERVER` in Controller classes - **Requires:** Symfony `Request` object methods - **Example:** @@ -115,7 +153,7 @@ These additional rules enforce Symfony-inspired MVC patterns in OpenEMR modules. $filter = $request->query->get('filter'); ``` -#### NoLegacyResponseMethodsRule +**NoLegacyResponseMethodsRule** - **Forbids:** `header()`, `http_response_code()`, `die()`, `exit`, direct `echo` in controllers - **Requires:** Symfony `Response` objects - **Example:** @@ -133,7 +171,7 @@ These additional rules enforce Symfony-inspired MVC patterns in OpenEMR modules. throw new ModuleException('Error'); ``` -#### ControllersMustReturnResponseRule +**ControllersMustReturnResponseRule** - **Forbids:** Controller methods returning `void` or no return type - **Requires:** Return type declaration of `Response` or subclass - **Example:** @@ -151,24 +189,6 @@ These additional rules enforce Symfony-inspired MVC patterns in OpenEMR modules. } ``` -## Rule Configuration - -You can selectively enable rules by creating your own configuration: - -```neon -# Custom phpstan.neon -services: - # Just database rules - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenFunctionsRule - tags: - - phpstan.rules.rule - - # Just module controller rules - - class: OpenCoreEMR\PHPStan\Rules\Module\NoSuperGlobalsInControllersRule - tags: - - phpstan.rules.rule -``` - ## Baselines If you're adding these rules to an existing codebase, generate a baseline to exclude existing violations: @@ -179,30 +199,15 @@ vendor/bin/phpstan analyze --generate-baseline New code will still be checked against all rules. -## Migration Guides - -See [MIGRATION_GUIDE.md](MIGRATION_GUIDE.md) for detailed migration patterns for each rule. - ## Development ### Running Tests ```bash -# Install dependencies composer install - -# Run PHPStan on the rules themselves -vendor/bin/phpstan analyze +vendor/bin/phpunit ``` -## Contributing - -Contributions are welcome! Please: - -1. Follow existing code style and patterns -2. Add tests for new rules -3. Update documentation - ## License GNU General Public License v3.0 or later. See LICENSE diff --git a/composer.json b/composer.json index 1fcdc0f..464c510 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,9 @@ "require": { "php": "^8.2", "nikic/php-parser": "^5.0", - "phpstan/phpstan": "^2.0" + "phpstan/phpstan": "^2.0", + "phpstan/phpstan-deprecation-rules": "^2.0", + "spaze/phpstan-disallowed-calls": "^4.0" }, "require-dev": { "ergebnis/composer-normalize": "^2.42", @@ -39,12 +41,13 @@ "config": { "allow-plugins": { "ergebnis/composer-normalize": true - } + }, + "sort-packages": true }, "extra": { "phpstan": { "includes": [ - "module.neon" + "extension.neon" ] } }, diff --git a/core.neon b/core.neon deleted file mode 100644 index 5978d85..0000000 --- a/core.neon +++ /dev/null @@ -1,57 +0,0 @@ -# PHPStan Extension for OpenEMR Core -# -# This configuration includes rules for OpenEMR core development: -# - Database: Forbid legacy SQL functions, methods, and Laminas-DB -# - Http: Forbid raw curl_* functions -# - Globals: Forbid direct $GLOBALS access -# - Testing: Forbid @covers annotations -# -# Include this in your phpstan.neon with: -# includes: -# - vendor/opencoreemr/openemr-phpstan-rules/core.neon -# -# @author Michael A. Smith -# @copyright Copyright (c) 2025 OpenCoreEMR Inc -# @license https://github.com/openCoreEMR/openemr-phpstan-rules/blob/main/LICENSE GNU General Public License 3 - -services: - # Database Rules - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenFunctionsRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenClassesRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenMethodsRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenStaticMethodsRule - tags: - - phpstan.rules.rule - - # Http Rules - - class: OpenCoreEMR\PHPStan\Rules\Http\ForbiddenCurlFunctionsRule - tags: - - phpstan.rules.rule - - # Globals Rules - - class: OpenCoreEMR\PHPStan\Rules\Globals\ForbiddenGlobalsAccessRule - tags: - - phpstan.rules.rule - - # Testing Rules - - class: OpenCoreEMR\PHPStan\Rules\Testing\NoCoversAnnotationRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Testing\NoCoversAnnotationOnClassRule - tags: - - phpstan.rules.rule - - # Exception Handling Rules - - class: OpenCoreEMR\PHPStan\Rules\ExceptionHandling\CatchThrowableNotExceptionRule - tags: - - phpstan.rules.rule diff --git a/extension.neon b/extension.neon index 9a76e97..a418125 100644 --- a/extension.neon +++ b/extension.neon @@ -1,15 +1,127 @@ # PHPStan Extension for OpenEMR # -# This is an alias for module.neon for backward compatibility. -# By default, includes all rules for comprehensive analysis. +# Rules for OpenEMR core and module development: +# - Database: Forbid legacy SQL functions, methods, and Laminas-DB +# - Http: Forbid raw curl_* functions +# - Globals: Forbid direct $GLOBALS access +# - Testing: Forbid @covers annotations +# - LegacyPHP: Forbid call_user_func/call_user_func_array +# - Module: Controller request/response handling +# - Deprecation: Report usage of deprecated code (via phpstan/phpstan-deprecation-rules) # # Include this in your phpstan.neon with: # includes: # - vendor/opencoreemr/openemr-phpstan-rules/extension.neon # # @author Michael A. Smith -# @copyright Copyright (c) 2025 OpenCoreEMR Inc +# @copyright Copyright (c) 2026 OpenCoreEMR Inc # @license https://github.com/openCoreEMR/openemr-phpstan-rules/blob/main/LICENSE GNU General Public License 3 +# +# Note: spaze/phpstan-disallowed-calls and phpstan/phpstan-deprecation-rules are +# automatically loaded by PHPStan's extension-installer. Do not include them +# explicitly to avoid duplicate service registration errors. + +parameters: + disallowedFunctionCalls: + # Legacy call_user_func functions + - + function: 'call_user_func()' + message: 'Use first-class callables instead of call_user_func().' + errorTip: 'Example: $callable = $object->method(...); $callable($args);' + - + function: 'call_user_func_array()' + message: 'Use first-class callables instead of call_user_func_array().' + errorTip: 'Example: $callable = someFunction(...); $callable(...$args);' + + # Legacy SQL functions (use QueryUtils or DatabaseQueryTrait instead) + - + function: 'sqlQuery()' + message: 'Use QueryUtils::querySingleRow() or QueryUtils::fetchRecords() instead of sqlQuery().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlStatement()' + message: 'Use QueryUtils::sqlStatementThrowException() or QueryUtils::fetchRecords() instead of sqlStatement().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlInsert()' + message: 'Use QueryUtils::sqlInsert() instead of sqlInsert().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlFetchArray()' + message: 'Use QueryUtils::fetchRecords() or QueryUtils::fetchArrayFromResultSet() instead of sqlFetchArray().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlBeginTrans()' + message: 'Use QueryUtils::startTransaction() instead of sqlBeginTrans().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlCommitTrans()' + message: 'Use QueryUtils::commitTransaction() instead of sqlCommitTrans().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlRollbackTrans()' + message: 'Use QueryUtils::rollbackTransaction() instead of sqlRollbackTrans().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlStatementNoLog()' + message: 'Use QueryUtils::fetchRecordsNoLog() instead of sqlStatementNoLog().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlStatementThrowException()' + message: 'Use QueryUtils::sqlStatementThrowException() instead of global sqlStatementThrowException().' + errorTip: 'Or use DatabaseQueryTrait in your class' + - + function: 'sqlQueryNoLog()' + message: 'Use QueryUtils::querySingleRow() instead of sqlQueryNoLog().' + errorTip: 'Or use DatabaseQueryTrait in your class' + +services: + # Database Rules + - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenClassesRule + tags: + - phpstan.rules.rule + + - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenMethodsRule + tags: + - phpstan.rules.rule + + - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenStaticMethodsRule + tags: + - phpstan.rules.rule + + # Http Rules + - class: OpenCoreEMR\PHPStan\Rules\Http\ForbiddenCurlFunctionsRule + tags: + - phpstan.rules.rule + + # Globals Rules + - class: OpenCoreEMR\PHPStan\Rules\Globals\ForbiddenGlobalsAccessRule + tags: + - phpstan.rules.rule + + # Testing Rules + - class: OpenCoreEMR\PHPStan\Rules\Testing\NoCoversAnnotationRule + tags: + - phpstan.rules.rule + + - class: OpenCoreEMR\PHPStan\Rules\Testing\NoCoversAnnotationOnClassRule + tags: + - phpstan.rules.rule + + # Exception Handling Rules + - class: OpenCoreEMR\PHPStan\Rules\ExceptionHandling\CatchThrowableNotExceptionRule + tags: + - phpstan.rules.rule + + # Module Rules + - class: OpenCoreEMR\PHPStan\Rules\Module\NoSuperGlobalsInControllersRule + tags: + - phpstan.rules.rule + + - class: OpenCoreEMR\PHPStan\Rules\Module\NoLegacyResponseMethodsRule + tags: + - phpstan.rules.rule -includes: - - module.neon + - class: OpenCoreEMR\PHPStan\Rules\Module\ControllersMustReturnResponseRule + tags: + - phpstan.rules.rule diff --git a/module.neon b/module.neon deleted file mode 100644 index 39bf60c..0000000 --- a/module.neon +++ /dev/null @@ -1,55 +0,0 @@ -# PHPStan Extension for OpenEMR Module Development -# -# This configuration includes all rules for OpenEMR module development: -# - All core rules (database, globals, testing) -# - Module-specific rules (controllers, request/response handling) -# -# Include this in your phpstan.neon with: -# includes: -# - vendor/opencoreemr/openemr-phpstan-rules/module.neon -# -# @author Michael A. Smith -# @copyright Copyright (c) 2025 OpenCoreEMR Inc -# @license https://github.com/openCoreEMR/openemr-phpstan-rules/blob/main/LICENSE GNU General Public License 3 - -services: - # Database Rules - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenFunctionsRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Database\ForbiddenClassesRule - tags: - - phpstan.rules.rule - - # Globals Rules - - class: OpenCoreEMR\PHPStan\Rules\Globals\ForbiddenGlobalsAccessRule - tags: - - phpstan.rules.rule - - # Testing Rules - - class: OpenCoreEMR\PHPStan\Rules\Testing\NoCoversAnnotationRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Testing\NoCoversAnnotationOnClassRule - tags: - - phpstan.rules.rule - - # Exception Handling Rules - - class: OpenCoreEMR\PHPStan\Rules\ExceptionHandling\CatchThrowableNotExceptionRule - tags: - - phpstan.rules.rule - - # Module-Specific Rules - - class: OpenCoreEMR\PHPStan\Rules\Module\NoSuperGlobalsInControllersRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Module\NoLegacyResponseMethodsRule - tags: - - phpstan.rules.rule - - - class: OpenCoreEMR\PHPStan\Rules\Module\ControllersMustReturnResponseRule - tags: - - phpstan.rules.rule diff --git a/src/Rules/Database/ForbiddenFunctionsRule.php b/src/Rules/Database/ForbiddenFunctionsRule.php deleted file mode 100644 index 9cf2426..0000000 --- a/src/Rules/Database/ForbiddenFunctionsRule.php +++ /dev/null @@ -1,76 +0,0 @@ - - * @copyright Copyright (c) 2025 OpenCoreEMR Inc - * @license https://github.com/openCoreEMR/openemr-phpstan-rules/blob/main/LICENSE GNU General Public License 3 - */ - -namespace OpenCoreEMR\PHPStan\Rules\Database; - -use PhpParser\Node; -use PhpParser\Node\Expr\FuncCall; -use PhpParser\Node\Name; -use PHPStan\Analyser\Scope; -use PHPStan\Rules\Rule; -use PHPStan\Rules\RuleErrorBuilder; - -/** - * @implements Rule - */ -class ForbiddenFunctionsRule implements Rule -{ - /** - * Map of forbidden functions to their error messages - */ - private const FORBIDDEN_FUNCTIONS = [ - 'sqlQuery' => 'Use QueryUtils::querySingleRow() or QueryUtils::fetchRecords() instead of sqlQuery().', - 'sqlStatement' => 'Use QueryUtils::sqlStatementThrowException() or QueryUtils::fetchRecords() instead of sqlStatement().', - 'sqlInsert' => 'Use QueryUtils::sqlInsert() instead of sqlInsert().', - 'sqlFetchArray' => 'Use QueryUtils::fetchRecords() or QueryUtils::fetchArrayFromResultSet() instead of sqlFetchArray().', - 'sqlBeginTrans' => 'Use QueryUtils::startTransaction() instead of sqlBeginTrans().', - 'sqlCommitTrans' => 'Use QueryUtils::commitTransaction() instead of sqlCommitTrans().', - 'sqlRollbackTrans' => 'Use QueryUtils::rollbackTransaction() instead of sqlRollbackTrans().', - 'sqlStatementNoLog' => 'Use QueryUtils::fetchRecordsNoLog() instead of sqlStatementNoLog().', - 'sqlStatementThrowException' => 'Use QueryUtils::sqlStatementThrowException() instead of global sqlStatementThrowException().', - 'sqlQueryNoLog' => 'Use QueryUtils::querySingleRow() instead of sqlQueryNoLog().', - ]; - - public function getNodeType(): string - { - return FuncCall::class; - } - - /** - * @param FuncCall $node - * @return array<\PHPStan\Rules\RuleError> - */ - public function processNode(Node $node, Scope $scope): array - { - if (!($node->name instanceof Name)) { - return []; - } - - $functionName = $node->name->toString(); - - // Only check if it's a forbidden function - if (!isset(self::FORBIDDEN_FUNCTIONS[$functionName])) { - return []; - } - - $message = self::FORBIDDEN_FUNCTIONS[$functionName]; - - return [ - RuleErrorBuilder::message($message) - ->identifier('openemr.deprecatedSqlFunction') - ->tip('Or use DatabaseQueryTrait in your class') - ->build() - ]; - } -} diff --git a/tests/Rules/Database/ForbiddenFunctionsRuleTest.php b/tests/Rules/Database/ForbiddenFunctionsRuleTest.php deleted file mode 100644 index cd4059f..0000000 --- a/tests/Rules/Database/ForbiddenFunctionsRuleTest.php +++ /dev/null @@ -1,75 +0,0 @@ - - * @copyright Copyright (c) 2026 OpenCoreEMR Inc - * @license https://github.com/openCoreEMR/openemr-phpstan-rules/blob/main/LICENSE GNU General Public License 3 - */ - -namespace OpenCoreEMR\PHPStan\Tests\Rules\Database; - -use OpenCoreEMR\PHPStan\Rules\Database\ForbiddenFunctionsRule; -use PHPStan\Rules\Rule; -use PHPStan\Testing\RuleTestCase; - -/** - * @extends RuleTestCase - */ -class ForbiddenFunctionsRuleTest extends RuleTestCase -{ - protected function getRule(): Rule - { - return new ForbiddenFunctionsRule(); - } - - public function testForbiddenFunctions(): void - { - $tip = 'Or use DatabaseQueryTrait in your class'; - $this->analyse([__DIR__ . '/data/forbidden-functions.php'], [ - [ - 'Use QueryUtils::querySingleRow() or QueryUtils::fetchRecords() instead of sqlQuery().', - 5, - $tip, - ], - [ - 'Use QueryUtils::sqlStatementThrowException() or QueryUtils::fetchRecords() instead of sqlStatement().', - 6, - $tip, - ], - [ - 'Use QueryUtils::sqlInsert() instead of sqlInsert().', - 7, - $tip, - ], - [ - 'Use QueryUtils::fetchRecords() or QueryUtils::fetchArrayFromResultSet() instead of sqlFetchArray().', - 8, - $tip, - ], - [ - 'Use QueryUtils::startTransaction() instead of sqlBeginTrans().', - 9, - $tip, - ], - [ - 'Use QueryUtils::commitTransaction() instead of sqlCommitTrans().', - 10, - $tip, - ], - [ - 'Use QueryUtils::rollbackTransaction() instead of sqlRollbackTrans().', - 11, - $tip, - ], - ]); - } - - public function testAllowedFunctions(): void - { - $this->analyse([__DIR__ . '/data/allowed-functions.php'], []); - } -} diff --git a/tests/Rules/Database/data/allowed-functions.php b/tests/Rules/Database/data/allowed-functions.php deleted file mode 100644 index ae8132e..0000000 --- a/tests/Rules/Database/data/allowed-functions.php +++ /dev/null @@ -1,12 +0,0 @@ - $x * 2, [1, 2, 3]); -json_encode(['key' => 'value']); - -// Method calls are allowed (only global functions are forbidden) -$db->sqlQuery('SELECT * FROM users'); -$utils->sqlStatement('UPDATE users SET name = ?', ['John']); diff --git a/tests/Rules/Database/data/forbidden-functions.php b/tests/Rules/Database/data/forbidden-functions.php deleted file mode 100644 index 0d53ed8..0000000 --- a/tests/Rules/Database/data/forbidden-functions.php +++ /dev/null @@ -1,11 +0,0 @@ -