chore: increase PHPStan level from 3 to 5 #314
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add ignore pattern for "Trait used zero times" errors (framework traits provided for userland but not used internally) - Fix redundant nullsafe operators where type is guaranteed non-null: - NotificationSender: $events is non-nullable EventDispatcherInterface - SanctumGuard: short-circuit logic guarantees truthy value Reduces errors from 218 to 195. More fixes to follow.
Adds literal string union type annotations to hasRecipient and hasEnvelopeRecipient methods, allowing PHPStan to verify the match expression is exhaustive for the 5 valid property values.
- Redis: Ignore finally.exitPoint (fix in separate PR) - FileStore: Ignore catch.neverThrown (exception thrown inside closure) - TestResponseAssert: Ignore catch.neverThrown (dynamic method call) - NestedSet Collection: Ignore foreach.emptyArray (complex groupBy inference)
- Container: Add class-string PHPDoc for callback type keys - RouteDependency: Add class-string PHPDoc for callback array keys - RedisWorkloadRepository: Remove unused $masters property (dead code) - Mailable: CS fixer formatting
Remove dead code that PHPStan correctly identified as unreachable: - Gate: Remove redundant if check (getMethod always returns or throws) - Mailer: Remove unreachable throw after exhaustive type checks - FakeProcessResult: Remove unreachable return after exhaustive type checks - HttpClientWatcher: Remove unreachable return after try-catch Fix PHPDoc annotations for nullable callables: - AuthenticationException: @var callable -> @var null|callable - Worker: @var callable -> @var null|callable Add temporary ignore for logic bug (fix in separate PR): - ServiceProvider: pathsForProviderOrGroup always returns array, not null
Fixes: - Str::isUuid: Add 'nil' to PHPDoc type (was missing, code checks for it) - QueueFake: Remove dead code (object !== string always false) - RedisStore: Ignore NaN detection idiom ($v === $v is only false for NaN) Temporary ignores for bugs fixed in separate PRs: - FilesystemManager: Operator precedence bug - StartSession: Operator precedence bug - TestResponseAssert: getHeader returns array not string Ignores for defensive/platform-specific code: - BcryptHasher: PHP 7 defensive code (PHP 8 throws instead) - ArgonHasher: Platform-specific PASSWORD_ARGON2_PROVIDER constant
Add @var annotation to help PHPStan understand that the Collection chain produces a list (integer-indexed array). Simplify the condition by removing redundant isset() check - after the empty($types) guard, a non-empty list always has index 0.
- Model.php: Add ignore for defensive backtrace handling
- FoundationServiceProvider.php: Add ignore for defensive backtrace handling
- Reflector.php: Simplify redundant ?? after isset check (short-circuit guarantees value exists)
- ValidatesAttributes.php: Fix PHPDoc from array{0: string} to array{0?: string} (confirmed rule can have 0 or 1 parameters)
- ArgonHasher.php: Add booleanAnd.alwaysFalse to existing platform-specific ignore - Email.php, File.php, Password.php: Add ignores for instanceof.alwaysTrue and booleanAnd.alwaysFalse (PHPStan's callable|static type narrowing doesn't account for closures not being class instances) - StartSession.php: Add booleanAnd.alwaysFalse to existing bug ignore
InteractsWithQueue: - Remove unreachable else branch in fail() method - After type narrowing, $exception is Throwable|null, so the instanceof Throwable || is_null() condition is always true - Remove unused InvalidArgumentException import ExcludeIf/ProhibitedIf: - Change parameter type from mixed to bool|Closure to match property type - Remove redundant runtime validation (PHP type system handles it) - Remove unused InvalidArgumentException import - Update tests to expect TypeError instead of InvalidArgumentException (both correctly reject invalid types, just different exception)
- CoreMiddleware: Remove incorrect @var annotation that asserted type before validation (let instanceof narrow the mixed return from getAttribute) - HasPermission/HasRole: Add ignores for match expression type narrowing (PHPStan doesn't track types perfectly across match arms) - Pipe/Pool: Add ignores for defensive validation of collection elements - QueryWatcher: Add ignore for PDO check with fallback code
Simplify ternary operators that always evaluate to false: - Lines 43-44 use `$defaultMethod ?: '__invoke'` but at this point $defaultMethod is guaranteed to be falsy (line 32 returns early when truthy) - Replace with just `'__invoke'` since that's always the result
- Factory: Fix PHPDoc to allow null for $modelNameResolver, $factoryNameResolver - Sleep/EventFake: Add ignores for intentional assertTrue(true) assertion counting
- Remove redundant double ?? [] patterns in ProviderConfig, RegisterProviders, RegisterFacades, VendorPublishCommand - Fix operator precedence issues: (int) $x ?? 0 → (int) ($x ?? 0) in SqsQueue, $a . $b ?? '' → $a . ($b ?? '') in Event - Remove dead ?? fallbacks after string casts/functions that never return null - Add ignore for defensive fallback in exception handler
- Progress: Add ignore for match arm always-true (correct exhaustive logic) - Listener: Add ignore for intentional infinite while(true) loop - Reflector: Remove redundant is_string check (already validated above) - ExtractProperties: Add ignore for PHP version check - MessageSelector: Add ignore for Arabic plural formula (modulo <= 99) - Prompt/ValidatesAttributes: Add ignores for defensive null checks - EventFake: Add ignore for intentional assertTrue(false) test failure
- Authorize: Remove dead is_null() check on array parameter, fix return type - BroadcastManager: Remove redundant is_null() on string parameter - CacheManager: Remove redundant is_null() on string parameter - SwooleTableManager: Remove redundant is_null() on string parameter - DatabaseStore: Remove no-op .map() converting arrays to objects (already objects)
- Container: Use hasContainer() instead of is_null(getContainer()) - CallQueuedListener: Add ignore for defensive deserialization check - Kernel: Fix PHPDoc to include null for optional upload files - TestResponseAssert: Remove dead is_string() check on Throwable parameter - ContinueSupervisorCommand/PauseSupervisorCommand: Check === 0 instead of is_null after (int) cast - MailManager: Fix return type to ?array (config lookup can return null) - MailMessage: Make priority nullable with default null - QueueManager: Remove redundant is_null() on string parameter - RedisQueue: Remove dead if wrapper (retryAfter always int)
- VendorPublishCommand: Add ignore for defensive choice() null check - SanctumGuard: Remove dead null check on non-nullable provider - DataObject: Remove dead null check (already guarded by is_array check) - Manager: Remove dead null check (getDefaultDriver returns string) - ServiceProvider: Add ignore for known logic bug (fix in separate PR)
Remove redundant type checks where PHP's type system already guarantees the type: - Remove dead is_string/is_array/is_int checks after type narrowing - Remove method_exists for methods that always exist (PHP 8+, interface contracts) - Remove property_exists checks on typed properties - Add ignores for legitimate defensive checks (platform compat, PHPDoc validation)
Complete PHPStan level 4 compliance: - Remove dead type checks where PHP types already guarantee the type - Add ignores for trait flexibility patterns (property_exists, method_exists) - Add ignores for interface optional methods (@method PHPDoc) - Add ignores for defensive checks on external/event data - Simplify telescope/cache watchers using actual property types
- Change HasLaravelStyleCommand trait to use Application instead of ContainerInterface - this correctly reflects that the container in Hypervel is always an Application instance - Update RetryCommand to match the trait's Application type - Fix ScheduleListCommand to iterate over lines instead of passing array - Add ignore for Redis zadd signature mismatch (Hyperf proxy differs)
- Console commands: Cast option values to int for subHours/plural calls - Filesystem: Add ignores for FtpAdapter/SftpAdapter interface quirk - Foundation: Fix str_replace type, array_filter callback, add type assertion - Horizon: Change Exception to Throwable for job failures (matches Laravel) - Horizon: Cast Redis timestamps to string, add higher-order proxy ignore - Prompts: Add ignores for intentional array_values on lists - Validation: Add ignores for defensive array_values/array_filter - Cors: Add ignore for bug (to be fixed in separate PR)
- HeaderUtils: Add ignore for implode type (conditional type narrowing) - ParsesLogConfiguration: Add @return PHPDoc for Monolog level constants - MailManager: Add @var for EsmtpTransport factory return type - BaseRelation: Add @var for nested-set QueryBuilder type - HasPermission: Add ignore for Permission contract in Collection::map
Contributor
Author
|
@albertcht All done - level 5 is passing now! I’ve updated the PR description with more details too. |
binaryfire
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current PHPStan level is 3. I think that's too low for framework code (I've already found a few bugs that would've been caught at level 5). For comparison, Hyperf uses level 6 and Tempest uses level 5. I think level 5 would be a good middleground for us. This PR increases the static analysis strictness from level 3 to level 5, fixing approximately 250+ errors along the way.
Background
Much of Hypervel's codebase is ported from Laravel, which has accumulated significant amounts of dead and redundant code over the years. PHPStan level 5 is strict enough to detect these issues:
is_string($value)where$valueis already typed as string??oris_null()on values that can never be nullmethod_exists()calls for interface methodselsebranches that can never execute based on type narrowingThese patterns likely accumulated in Laravel as PHP's type system evolved. Code written for PHP 5.x/7.x that used runtime type checks became redundant once proper type declarations were added, but the old checks were never cleaned up.
Categories of fixes
Removed type checks that PHPStan proved will always return the same value:
is_string(),is_array(),is_null(),is_int()on already-typed valuesmethod_exists()for methods guaranteed by interfacesinstanceofchecks that always passelse/catchblocksAdded explicit casts where PHP's type coercion was implicit:
$this->option()returns mixed, cast to int for numeric usage)ceil()returns float, cast to int for methods expecting integers)Added return type annotations to help PHPStan understand code:
Some "redundant" checks are intentionally defensive. These were kept with
@phpstan-ignoreannotations:@methodPHPDoc annotations in interfacesExceptiontoThrowablein Horizon's job failure handling to match Laravel's queue system (jobs can fail withError, not justException)I made separate PRs for several bug fixes as well.