chore(phpstan): climb to level 5 (argument types)#3566
Conversation
Level 5 turns on argument-type checking (every call's args vs the callee's param types).
Against the green level-4 tree this surfaced exactly 38 errors, all `argument.type`, thinly
spread (1-4 per file). No baseline — fixed forward. Triaged + fixed by category:
Genuine latent bugs (4):
- Ldap::getEmail/getSingleUser called ldap_error() on a connection handle statically known to be
`false` in the no-connection guard — a PHP 8 TypeError that crashed the failure path instead of
degrading. Now log + short-circuit (return ''/false; callers already handle both).
- DTO placement built a nested array into implode() (-> "Array.x"); flatten the path parts.
- StaticAsset Content-length used filesize() which is int|false; guard the false case.
Type-hint / docblock corrections (10): mergeCanvas $mergeId, getComments $orderByState,
getProjectsUserHasAccessTo $clientId int-ified; Notifications getAllNotifications $showNewOnly
bool; stale `@param false` docblocks on Mailer::setHtml / Users::getAll widened to bool;
Tickets getOpenUserTicketsThisWeekAndLater @param widened; RequestRateLimiter getHeaders $limit int.
Call-site casts/guards (12): Auth (string)time(); Timesheets (int) clientId/ticketFilter;
ChangeCurrentProject (int) id; InviteTeamStep guards createUserInvite()'s false; Jsonrpc
json_decode associative true (was JSON_OBJECT_AS_ARRAY int); MigrateCommand (string) cast;
I18n drops a stray bool arg; Ldap Log::error drops a stray int context.
Framework type-gaps (11): Symfony Cookie::withSameSite lowercased 'lax'/'strict' (Symfony
strtolowers anyway); cache TTLs passed a Carbon datetime -> new \DateInterval('P7D'/'P30D');
three narrow `@phpstan-ignore-next-line argument.type` for vendor contract looseness
(Sanctum model class-string, set_error_handler callback, RequestHandled Symfony-vs-Illuminate Response).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Raises PHPStan to level 5 (argument-type checking) and updates various call sites, signatures, and TTL handling across the codebase to satisfy stricter type expectations while also fixing several runtime edge cases uncovered by the new analysis level.
Changes:
- Bumps PHPStan configuration from level 4 to level 5 and resolves newly surfaced
argument.typefindings. - Normalizes argument types via casts / signature adjustments (e.g., bool vs int flags, int IDs, TTLs via
DateInterval). - Fixes a few concrete runtime hazards (e.g., LDAP no-connection path, DTO property path flattening, guarding
filesize()).
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/Domain/Widgets/Services/Widgets.php | Switches cache TTL to DateInterval for PSR-16 set() compatibility. |
| app/Domain/Users/Services/Users.php | Cleans up incorrect docblock param annotation. |
| app/Domain/Timesheets/Services/Timesheets.php | Casts string filters to int for repository argument types. |
| app/Domain/Tickets/Services/Tickets.php | Widens docblock type for optional $projectId. |
| app/Domain/Projects/Repositories/Projects.php | Tightens clientId parameter type/default for access query. |
| app/Domain/Projects/Controllers/ChangeCurrentProject.php | Normalizes project id parsing to (int) cast. |
| app/Domain/Plugins/Services/Registration.php | Uses DateInterval for cache TTLs; removes Carbon usage. |
| app/Domain/Notifications/Services/Notifications.php | Changes showNewOnly argument type to bool. |
| app/Domain/Ldap/Services/Ldap.php | Fixes no-connection failure path and logging argument types. |
| app/Domain/Help/Services/InviteTeamStep.php | Guards invite flow when user creation fails; casts id. |
| app/Domain/Comments/Repositories/Comments.php | Changes orderByState to int and uses strict comparison. |
| app/Domain/Blueprints/Repositories/Blueprints.php | Fixes mergeCanvas() signature/docblock type for merge id. |
| app/Domain/Auth/Services/Auth.php | Matches repository signature by casting time() to string. |
| app/Domain/Api/Services/I18n.php | Removes incorrect boolean default argument to Language::__(). |
| app/Domain/Api/Controllers/StaticAsset.php | Guards filesize() result before setting Content-Length. |
| app/Domain/Api/Controllers/Jsonrpc.php | Uses associative json_decode(..., true) for argument typing. |
| app/Core/UI/Theme.php | Normalizes Cookie SameSite strings to lowercase. |
| app/Core/Middleware/RequestRateLimiter.php | Casts rate-limit values and updates header helper signature. |
| app/Core/Mailer.php | Removes incorrect docblock param annotation. |
| app/Core/Language.php | Normalizes Cookie SameSite string to lowercase. |
| app/Core/Http/HttpKernel.php | Adds targeted PHPStan ignore for framework event dispatch typing gap. |
| app/Core/Exceptions/HandleExceptions.php | Adds targeted PHPStan ignore for set_error_handler callback typing. |
| app/Core/Domains/DTO.php | Fixes nested property-path assembly for attribute filtering. |
| app/Core/Auth/Tokens/SanctumServiceProvider.php | Adds targeted PHPStan ignore for Sanctum model type contract looseness. |
| app/Command/MigrateCommand.php | Casts install status before writing to console output. |
| .phpstan/phpstan.neon | Raises PHPStan level from 4 to 5. |
Comments suppressed due to low confidence (2)
app/Domain/Api/Controllers/Jsonrpc.php:117
get()decodes$params['params']and then checksif ($params == null), which will never be null (it’s always an array). Also, whenparamsis omitted,$paramsDecodedbecomes an array and is passed tojson_decode(), which throws aTypeErrorin PHP 8+. This makes GET JSON-RPC requests withoutparams(or with invalid base64/JSON) fail incorrectly.
$params['params'] = json_decode($params['params'], true);
// check if decode failed
if ($params == null) {
return $this->returnParseError('JSON is invalid and was not able to be parsed');
app/Core/Middleware/RequestRateLimiter.php:142
X-RateLimit-Limitis currently populated with the current attempt count ($this->limiter->attempts($key)), not the configured limit. This makes the header misleading for clients trying to respect rate limiting.
return [
'X-RateLimit-Remaining' => $this->limiter->retriesLeft($key, $limit),
'X-RateLimit-Retry-After' => $this->limiter->availableIn($key),
'X-RateLimit-Limit' => $this->limiter->attempts($key),
'Retry-After' => $this->limiter->availableIn($key),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $size = filesize($response->getFile()->getPathname()); | ||
| $response->headers->set('Content-length', (string) ($size !== false ? $size : 0)); | ||
|
|
There was a problem hiding this comment.
Fixed in 49ad5d4 — now only sets Content-length when filesize() returns an int; on failure it's omitted and BinaryFileResponse computes it during prepare(). No more bogus 0-length body.
| if ($installStatus !== true) { | ||
| $io->text($installStatus); | ||
| $io->text((string) $installStatus); | ||
|
|
||
| return Command::FAILURE; |
There was a problem hiding this comment.
Fixed in 49ad5d4 — setupDB() returns bool, so (string) false was printing an empty line. Replaced with $io->error('Database installation failed. Check the application logs for details.') (setupDB logs the underlying cause).
…nt-length - StaticAsset: only set Content-length when filesize() succeeds; on failure let BinaryFileResponse compute it rather than advertising a 0-length body (which clients/ proxies may treat as empty even though the file still streams). - MigrateCommand: setupDB() returns bool, so (string) false printed nothing on failure; emit a concrete error pointing CLI users at the logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follows #3565 (level 4, now merged). Continues the campaign: #3559 (L2) → #3561 (L3) → #3565 (L4) → this (L5).
Level 5 enables argument-type checking (every call's args vs the callee's param types). Against the green level-4 tree it surfaced 38 errors, all
argument.type, spread 1–4 per file. Fixed forward, no baseline.Breakdown (all 38 triaged before fixing)
The 4 real bugs
Ldap::getEmail/getSingleUsercalledldap_error()on a connection handle statically known to befalsein the no-connection guard — a PHP 8TypeErrorthat crashed the failure path instead of degrading. Now logs and short-circuits (return ''/false; callers already handle both).DTObuilt a nested array intoimplode()(→"Array.x"+ warning) for nested property paths; now flattens the path parts in the correct order.StaticAssetsetContent-lengthfromfilesize()which isint|false; thefalsecase is now guarded.Framework gaps
Symfony
Cookie::withSameSitenow gets lowercase'lax'/'strict'(itstrtolowers anyway — no behavior change); cache TTLs that passed an absoluteCarbondatetime now pass an equivalentnew \DateInterval('P7D'/'P30D'); three narrow@phpstan-ignore-next-line argument.typefor genuine vendor contract looseness (Sanctum modelclass-string,set_error_handlercallback return,RequestHandledSymfony-vs-IlluminateResponse).Verification
./vendor/bin/phpstan analyse -c .phpstan/phpstan.neon→[OK] No errorsatlevel: 5🤖 Generated with Claude Code