Skip to content

chore(phpstan): raise static analysis to level 2 (429→0, fix-forward)#3559

Merged
marcelfolaron merged 7 commits into
masterfrom
chore/phpstan-level-2
Jun 20, 2026
Merged

chore(phpstan): raise static analysis to level 2 (429→0, fix-forward)#3559
marcelfolaron merged 7 commits into
masterfrom
chore/phpstan-level-2

Conversation

@marcelfolaron

Copy link
Copy Markdown
Collaborator

Raise PHPStan from level 1 → 2

Climbs static analysis one level (config level: 12), fix-forward with no baseline — matching the level 0→1 precedent. make phpstan is green at level 2 (838 files, 0 errors).

The headline number was misleading: clean master had 429 level-2 findings, but they collapsed to a handful of patterns rather than 429 independent problems.

How the 429 were resolved

Systemic (~58% of all findings, ~3 changes):

  • DispatchesEvents trait: set_class_context()/get_function_context() private staticprotected static. The trait is mixed into ~60 classes, so static:: calls to a private method were reported once per using class (~112 reports, one defect).
  • Carbon date/time macros: resolved via Carbon's official PHPStan MacroExtension (vendor/nesbot/carbon/extension.neon, now included), fed by a CarbonImmutable::mixin(new CarbonMacros) registration in .phpstan/bootstrap.php. Deliberately not stubbed — stubbing Carbon\CarbonInterface clobbers Carbon's own @method API.
  • .phpstan/stubs/laravel-gaps.stub (new): declares concrete Laravel methods our code reaches through looser contracts (no Larastan) — ConnectionInterface::getDriverName/getPdo, Filesystem::url/mimeType, Auth Factory/Guard, Foundation\Application, Cache\Repository::lock, Socialite\Provider, Carbon::addRealMinutes.

Mechanical sweep (~100 findings, 47 files): stale @param names, @return/@param/@var vs native-type mismatches, malformed docblocks, and namespace typos — most notably \Leantime\Core\Bootstrap\Application\Leantime\Core\Application (that namespace never existed) and the correct BindingResolutionException / HttpResponseException FQCNs.

🐛 Real bugs surfaced by level 2 (fixed here, not just type noise)

  • session()->exist(...)exists() — the typo'd method doesn't exist (would fatal on the LDAP-import path).
  • Missing DateTimeHelper::parseDb24hTime()Format's FromFormat::Db24hTime case called a method that never existed; added it.
  • Profile pictures were never deletedsetPicture() is void but was used in ... && $this->userRepo->setPicture(...) && $oldPicture, so the branch was always falsy. Now returns bool.
  • ApiGuard::id/WebGuard::id use the Authenticatable contract's getAuthIdentifier() instead of ->id.
  • A handful of call sites passed extra arguments PHP was silently dropping (a lost 'open' project-status filter, a dropped client filter, etc.) — removed to match the real signatures.

⚠️ Flagged for review (FIXME(phpstan-l2) in the diff)

  • Files\Repositories\Files::upload() was calling upload($file, $newname, false) but the method is (UploadedFile $file, $disk = 'default') — the filename was being passed as the disk name. Changed to upload($file); the surrounding $values still use the caller's own $newname/$realName, which may not match the actually-stored file. Please confirm the intended disk + name handling.
  • Entityrelations::saveRelationship/getRelationshipByEntity call Setting::saveSetting/getSetting with 6/4 args against 2/1-2-arg signatures — the methods are broken (relationship data was silently dropped) and the injected EntityrelationRepository has no matching method. Trimmed to preserve current runtime behavior; the relationship-storage design needs rebuilding.

Notes

  • app/Plugins/* is a private submodule not checked out in CI, so this only covers core/domain (matches CI exactly).
  • Runtime-affecting changes (file upload, setPicture, guard(), session()->exists) should be validated with make unit-test / make acceptance-test before merge.
  • Level 3 (+46 findings, incl. an auth stdClassAuthenticatable change that touches the bearer/sanctum-sensitive UserProvider) is planned as a separate stacked PR.

🤖 Generated with Claude Code

marcelfolaron and others added 5 commits June 20, 2026 16:57
Centralized fixes that clear ~66% of level-2 findings with no per-call-site churn:

- DispatchesEvents trait: set_class_context()/get_function_context() private→protected
  static. The trait is mixed into ~60 classes, so static:: calls to a private method
  produced one staticClassAccess.privateMethod per using class (~112 reports, 1 defect).
- Carbon date/time macros: resolved via Carbon's official PHPStan MacroExtension
  (vendor extension.neon) fed by a mixin registration in .phpstan/bootstrap.php, instead
  of stubbing Carbon\CarbonInterface (which would clobber Carbon's own @method API).
- laravel-gaps.stub: declares concrete Laravel methods our code calls through looser
  contracts (ConnectionInterface::getDriverName, Filesystem::url, Auth Factory/Guard,
  Foundation\Application, Socialite Provider) — no Larastan, so vanilla PHPStan needs these.
- Language.php: static:: → self:: for private constants (class is final-by-usage, not extended).
- AuthenticateSession::guard() returns $this->auth->guard() (the actual Guard) instead of
  the AuthManager, matching upstream Laravel and keeping the return type honest.

Config still at level 1; this only adds stubs/extension so the level-2 bump lands clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PHPStan level 2 flagged genuine latent bugs (not just type noise):

- Users/Import: session()->exist(...) → exists() (the typo'd method does not exist;
  would fatal at runtime on the LDAP-import path).
- DateTimeHelper: add parseDb24hTime() — Format's FromFormat::Db24hTime case called a
  method that never existed (runtime fatal on that path). Mirrors parseUser24hTime in dbTimezone.
- Cast: narrow ReflectionType→ReflectionNamedType before ->getName() (union/intersection types
  have no getName()).
- Projects::getProjectProgress: cast DateInterval::format('%a') (string) to int before division.
- FileManager::convertPHPSizeToBytes: cast substr() to int before *= (string arithmetic).
- ApiGuard: gate getAPIKey() behind instanceof ApiRequest (getAPIKey lives on ApiRequest,
  not the IncomingRequest base; isApiRequest() is URI-based and doesn't prove the instance type).
- Install: Cache::store('installation')->flush() → clear() (flush() isn't on the Repository
  contract; clear() is the PSR-16 method that wipes the store).
- Setting/FileManagerInterface: declare getFileUrl() on the interface (concrete already has it).
- ExceptionHandler::isHttpException: @phpstan-assert-if-true so getStatusCode()/getHeaders()
  narrow correctly.
- Connector\Providers::getProvider(): return type provider-model → object (it returns provider
  service objects with connect()/getEntities(), never the model — the hint was wrong).
- laravel-gaps.stub: Carbon::addRealMinutes (Carbon-3 magic method, annotation dropped upstream).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
return.missing — add explicit `return null` where a typed method fell through:
EventDispatcher::dispatch, ApiGuard::id, WebGuard::id, Projects::getProjectName,
AuthenticateSession::redirectTo.

method.void — Users\Repositories::setPicture void→bool (+return true): it was used in
setProfilePicture's `&& $this->userRepo->setPicture(...) &&` chain, so the void/null return
made the branch ALWAYS falsy and the old profile picture was never deleted. Queue::addToQueue
/addJob no longer `return` the void addMessageToQueue() result.

method/property.nonObject — CheckPermissions narrows $request->route() with `instanceof Route`
before getControllerClass()/getActionMethod(); Integration guards `is_object()` before ->id.

arguments.count — extra args were silently dropped by PHP at runtime; removed to match the real
signatures (FLAGGED for review where a feature was being lost):
- Entityrelations saveRelationship/getRelationshipByEntity: passed 6/4 args to Setting's 2-arg
  saveSetting/getSetting — the methods are broken (relationship data dropped); trimmed + FIXME.
- Files upload: passed the md5 filename as the disk arg (would fail disk lookup); now upload($file)
  + FIXME on the encName mismatch.
- getProjectsUserHasAccessTo('open') ×3 and getAllClientsAvailableToUser(...,$client): dropped
  status/client filters that the method never accepted.
- quickAddTicket(...,projectId) and parseUserDateTime(...,FromFormat) extra args removed.

method.notFound — CarbonMacros setToUserTimezone/setToDbTimezone closures now return
CarbonImmutable (not CarbonInterface), so chained macro calls keep resolving.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Widgets\Models\Widget: declare `public bool $isNew` — the Widgets service set $widget->isNew
  dynamically (deprecated dynamic property in PHP 8.2+).
- Auth ApiGuard::id/WebGuard::id: use the Authenticatable contract's getAuthIdentifier()
  instead of ->id (the contract has no $id property).
- FacadeRule (our own PHPStan rule): PhpParser\Node\Name->parts → ->getParts() (the $parts
  property was removed in nikic/php-parser v5).
- Projects::notifyProjectUsers: @var Notification after dispatch_filter() (which returns mixed),
  restoring the type so ->module resolves.
- Timesheets: @var CarbonImmutable on a chain ending in startOfDay() (Carbon annotates it as
  returning the interface, where the macro can't resolve).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clears the remaining 102 level-2 findings across 47 files — all docblock / type-annotation /
namespace fixes, no runtime logic changes:

- parameter.notFound: stale @param names dropped/renamed to match real signatures.
- return/parameter/property.phpDocType: @return/@param/@var types reconciled with native types
  (or removed where redundant).
- class.notFound: @var \Leantime\Core\Bootstrap\Application → \Leantime\Core\Application (the
  Bootstrap segment never existed) in HandleExceptions/ExceptionHandler.
- throws.notThrowable: corrected exception FQCNs — \Illuminate\Contracts\Container\
  BindingResolutionException and \Illuminate\Http\Exceptions\HttpResponseException.
- phpDoc.parseError / varTag.variableNotFound / class.nameCase: malformed tags repaired,
  stale inline @var removed, Environment case corrected.
- laravel-gaps.stub: Cache\Repository::lock() (forwarded to a LockProvider store via __call;
  used by StartSession session locking).

`.phpstan/phpstan.neon` level 1 → 2. `make phpstan` is green at level 2 (838 files, 0 errors).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 20, 2026 21:46
@marcelfolaron marcelfolaron requested a review from a team as a code owner June 20, 2026 21:46
@marcelfolaron marcelfolaron requested review from broskees and removed request for a team June 20, 2026 21:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Raises PHPStan static analysis from level 1 to level 2 without a baseline, and resolves the newly surfaced issues through a combination of code fixes, signature/docblock alignment, and PHPStan configuration/stubs (including Carbon macro support).

Changes:

  • Bumps PHPStan to level 2 and adds bootstrap/stubs to correctly model runtime Laravel + Carbon behavior.
  • Fixes a set of real runtime bugs found by static analysis (e.g., session exists typo, missing DateTimeHelper method, auth guard id behavior, upload signature mismatches).
  • Cleans up type hints/docblocks and removes/adjusts calls with extra or mismatched arguments across multiple domains.

Reviewed changes

Copilot reviewed 73 out of 73 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
app/helpers.php Docblock param/return type alignment for helpers
app/Domain/Widgets/Models/Widget.php Adds transient isNew flag to widget model
app/Domain/Widgets/Controllers/WidgetManager.php Removes redundant phpdoc for typed property
app/Domain/Users/Repositories/Users.php Adjusts return types; setPicture() now returns bool
app/Domain/Users/Controllers/Import.php Fixes session()->exist() typo to exists()
app/Domain/Timesheets/Repositories/Timesheets.php Adds PHPStan type hint for timezone-adjusted Carbon
app/Domain/Tickets/Services/Tickets.php Docblock param rename; removes extra arg to date parsing
app/Domain/Tickets/Hxcontrollers/Milestones.php Fixes constructor param docblock type
app/Domain/Tickets/Controllers/ShowTicket.php Removes extra arg previously ignored/dropped by PHP
app/Domain/Tickets/Controllers/NewTicket.php Removes extra arg previously ignored/dropped by PHP
app/Domain/Reports/Services/Reports.php Fixes constructor docblock type
app/Domain/Reports/register.php Removes redundant local var phpdoc
app/Domain/Reactions/Services/Reactions.php Tightens boolfalse return docblock union
app/Domain/Reactions/Repositories/Reactions.php Tightens boolfalse return docblock union
app/Domain/Queue/Services/Queue.php Removes misleading return statements for void repo calls
app/Domain/Queue/register.php Removes redundant local var phpdoc
app/Domain/Projects/Services/Projects.php Cast fix, dispatch_filter typing hint, null return, doc tweaks
app/Domain/Projects/Repositories/Projects.php Removes duplicate/incorrect docblock; fixes return union
app/Domain/Plugins/Services/Plugins.php Removes redundant phpdoc; fixes model phpdoc type
app/Domain/Oidc/Services/Oidc.php Removes incorrect @return void docblocks
app/Domain/Oidc/Controllers/Callback.php Fixes HttpResponseException namespace
app/Domain/Notifications/Hxcontrollers/NewsBadge.php Fixes constructor param docblock
app/Domain/Notifications/Hxcontrollers/News.php Fixes constructor param docblock
app/Domain/Menu/Services/Menu.php Removes stale constructor docs; removes extra arg previously dropped
app/Domain/Menu/Repositories/Menu.php Removes stale constructor param doc
app/Domain/Menu/Composers/Menu.php Removes unused doc lines; keeps throws annotation
app/Domain/Install/Repositories/Install.php Uses cache clear() for installation store reset
app/Domain/Help/Services/Helper.php Fixes constructor param doc; clarifies return behavior
app/Domain/Help/Services/FirstTaskStep.php Removes extra arg previously dropped by PHP
app/Domain/Help/Hxcontrollers/HelperModal.php Updates init() docblock to match injected services
app/Domain/Files/Repositories/Files.php Updates upload call to match FileManager signature (fix-forward)
app/Domain/Entityrelations/Services/Entityrelations.php Trims broken extra args; documents design issue with FIXME
app/Domain/Connector/Services/Providers.php Fixes invalid return type (providerobject)
app/Domain/Connector/Controllers/Integration.php Guards property access when integration isn’t an object
app/Domain/Canvas/Services/Canvas.php Fixes docblock parameter type
app/Domain/Calendar/Services/Calendar.php Fixes docblock parameter types
app/Domain/Calendar/Repositories/Calendar.php Removes stale docs; tightens boolfalse union
app/Domain/Auth/Services/Auth.php Removes redundant phpdoc for typed property
app/Domain/Auth/Guards/WebGuard.php Uses getAuthIdentifier() in id() and returns null when absent
app/Domain/Auth/Guards/ApiGuard.php Adds ApiRequest check; uses getAuthIdentifier() in id()
app/Core/UI/Theme.php Removes incorrect phpdoc
app/Core/UI/Template.php Doc cleanup around response helpers
app/Core/UI/Composer.php Corrects compose() parameter docblock
app/Core/Support/Format.php Removes stale doc params for bytes formatting
app/Core/Support/DateTimeHelper.php Adds missing parseDb24hTime() method
app/Core/Support/Cast.php Fixes ReflectionNamedType handling; updates iterable doc
app/Core/Support/CarbonMacros.php Adjusts macro return types to CarbonImmutable
app/Core/Sessions/PathManifestRepository.php Fixes docblocks for params
app/Core/Middleware/Updated.php Fixes throws FQCN in docblocks
app/Core/Middleware/StartSession.php Corrects cache return type docblock
app/Core/Middleware/LoadPlugins.php Fixes throws FQCN in docblocks
app/Core/Middleware/Installed.php Fixes throws FQCN in docblocks
app/Core/Middleware/InitialHeaders.php Fixes throws FQCN in docblocks
app/Core/Middleware/AuthenticateSession.php Returns guard instance; returns null redirect path
app/Core/Middleware/AuthCheck.php Tightens return/throws docblocks
app/Core/Language.php Uses self:: for constants; doc cleanup
app/Core/Http/HtmxRequest.php Fixes array return type docblock
app/Core/Files/FileManager.php Casts parsed size suffix value to int
app/Core/Files/Contracts/FileManagerInterface.php Adds getFileUrl(); doc cleanup for disk params
app/Core/Exceptions/HandleExceptions.php Fixes Application namespace in phpdoc
app/Core/Exceptions/ExceptionHandler.php Fixes constructor docblock; adds phpstan assertion
app/Core/Events/EventDispatcher.php Adds explicit return null in dispatch()
app/Core/Events/DispatchesEvents.php Makes context helpers protected for trait users
app/Core/Controller/Frontcontroller.php Docblock alignment with actual parameters/returns
app/Core/Configuration/DefaultConfig.php Docblock type fixes for config properties
app/Core/Bootstrap/LoadConfig.php Docblock type clarifications
app/Core/Bootloader.php Removes stale doc param
app/Core/Auth/Permissions/CheckPermissions.php Ensures route is an Illuminate\Routing\Route before use
.phpstan/stubs/leantime-macros.stub Documents Carbon macro handling approach
.phpstan/stubs/laravel-gaps.stub Adds PHPStan stubs for contract/concrete Laravel gaps
.phpstan/Rules/FacadeRule.php Uses getParts() rather than accessing internal parts
.phpstan/phpstan.neon Includes Carbon extension, adds stub file, bumps level to 2
.phpstan/bootstrap.php Registers CarbonImmutable mixins for PHPStan macro resolution
Comments suppressed due to low confidence (1)

app/Domain/Files/Repositories/Files.php:293

  • Files::upload() now calls FileManagerInterface::upload($file, $disk) (which may rename files based on filesystem config), but the DB record is still written using the locally-generated $newname / $ext. This can desync DB metadata from the actual stored filename, breaking download/delete. Use the values returned by $this->fileManager->upload() when persisting the file record.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +23 to 26
if ($this->request instanceof ApiRequest && $this->request->isApiRequest()) {
$this->apiKey = $this->request->getAPIKey();
}
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 1858804: $apiKey now defaults to '' so empty($this->apiKey) is safe when the guard is built for a non-API request (and my added instanceof ApiRequest guard made that uninitialized path more reachable).

Comment on lines 72 to 79
public function id()
{
if ($this->user()) {
return $this->user()->id;
return $this->user()->getAuthIdentifier();
}

return null;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct — user() returns a stdClass at runtime (the (object) $apiUser cast; WebGuard has the same via AuthUser::retrieveById), so getAuthIdentifier() would fatal. Fixed in 1858804: both guards now read ->id off the object with an accurate @var. The proper conversion to a real Authenticatable is the planned level-3 auth pass.

Comment thread app/Domain/Users/Repositories/Users.php Outdated
Comment on lines 416 to 424
$this->connection->table('zp_user')
->where('id', $id)
->update([
'profileId' => $fileId,
'modified' => dtHelper()->dbNow()->formatDateTimeForDb(),
]);

return true;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1858804: setPicture() now returns ...->update([...]) > 0 so the return reflects whether the row was updated, rather than always true.

* @var string Send anonymous data <a href='https://docs.leantime.io/#/using-leantime/company-settings?id=telemetry' target='_blank'>More Info</a>.
* No personal identifieble data will be sent and it will be impossible for us to track individual users.
* @var bool Send anonymous data <a href='https://docs.leantime.io/#/using-leantime/company-settings?id=telemetry' target='_blank'>More Info</a>.
* No personal identifieble data will be sent and it will be impossible for us to track individual users.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1858804: 'identifieble' → 'identifiable'.

Pint auto-fixes on the files touched by the level-2 work: drop superfluous/empty PHPDoc
tags, remove the now-unused Carbon\CarbonInterface import in CarbonMacros, and normalize
spacing/braces. `pint --test` is clean (628 files) and `make phpstan` is still green at level 2.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@marcelfolaron

Copy link
Copy Markdown
Collaborator Author

CI status

  • pint — fixed in e792005 (Pint auto-format on the touched files; pint --test clean, make phpstan still green at level 2).
  • bearer-api — this failure is pre-existing on master, not introduced here. The API Tests (Bearer auth) workflow already fails on master at this branch's base commit 363a8ab84 (PR feat(events): class-based domain events + filters with legacy-string plugin bridge (Tickets pilot) #3503, feat(events)) with the identical failure: BearerApiCest::bearerAuthHonorsGatedReadselement with name 'email' was not found at Install.php:31 (Tests: 2, Assertions: 39, Failures: 1). The first bearer test (full authenticated JSON-RPC flow) passes here, so the auth-related changes in this PR are not the cause — the second test fails during its /install setup. That test needs a separate fix on master.

All other checks pass: phpstan (level 2 ✓), unit, acceptance, CodeQL, security audits, build.

@marcelfolaron

Copy link
Copy Markdown
Collaborator Author

Correction on bearer-api — it's flaky, and the PR is now fully green ✅

I was too quick to call the earlier bearer-api red "inherited from master" — that was a weak inference from a single push-run failure. The accurate picture, from two runs on this branch:

Commit Diff vs prior bearer-api
92188706c (sweep + level bump) ❌ fail
e792005 (Pint) style-only, no logic change ✅ pass

A purely cosmetic commit flipping fail→pass means the logic in this PR isn't the cause (a real break would also fail on the Pint commit). The failure is in BearerApiCest::_before's fresh-install step (fillField email on /install) — setup/DB-reset timing, not the bearer-auth/permission path the test guards. The test's own docblock notes "the Laravel container is not reliably bootstrapped against the test DB in the acceptance process."

All checks now pass on the latest commit (e792005): phpstan (level 2), pint, unit, acceptance, bearer-api, build, CodeQL, security audits.

- ApiGuard: give $apiKey a '' default so empty($this->apiKey) is safe when the guard is
  constructed for a non-API request (typed property was never initialized otherwise).
- ApiGuard::id / WebGuard::id: both guards' user() return a stdClass at runtime (the (object)
  cast / AuthUser::retrieveById), so calling getAuthIdentifier() would fatal. Read ->id off the
  object with an accurate @var instead. (The real Authenticatable conversion is the level-3 pass.)
- Users\Repositories::setPicture: return the actual update result (> 0) instead of always true,
  so the caller's control flow reflects whether the row was updated.
- DefaultConfig: spelling, identifieble → identifiable.

phpstan level 2 still green; pint clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 20, 2026 22:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 73 out of 73 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

app/Domain/Files/Repositories/Files.php:300

  • Files::upload() now delegates naming to FileManager::upload(), but still persists its own locally-generated $newname/$realName/$ext into zp_file. When FileManager is configured to rename files (or sanitizes the real name), the stored encName/extension can diverge from the actually-stored path, breaking later download/delete lookups that reconstruct the filename from DB fields. Persist the values returned by FileManager instead (and keep realName consistent with the rest of the UI by stripping the extension if needed).

@marcelfolaron marcelfolaron merged commit 84a5296 into master Jun 20, 2026
13 checks passed
@marcelfolaron marcelfolaron deleted the chore/phpstan-level-2 branch June 20, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants