chore(quality): clear phpstan baseline#285
Merged
Merged
Conversation
PHPStan flagged several call sites where implode() or natcasesort() received arrays of mixed values produced by parse_str() or json_decode(). The code assumed scalar/string values, which is what these APIs typically yield, but the static type system could not prove it. - JiraAuthenticationService::extractTokens() now funnels parsed values through a small recursive helper that coerces scalars to strings and drops non-stringable values, eliminating the implode mismatches without changing behaviour for well-formed responses. - JiraHttpClientService::extractErrorMessage() coerces decoded JSON values with array_map(strval-equivalent) before joining. - JiraOAuthApiService::getSubtickets() returns list<string> (subtaskKeys is already typed as such). - SubticketSyncService::syncProjectSubtickets() narrows its return type to list<string>, which lets natcasesort() and implode() type-check cleanly in the command. Removes 4 baseline entries (5 errors) without altering runtime semantics. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Replaces dynamic calls to PHPUnit\Framework\Assert::callback() with the static form, narrows post() callback payloads via inline array shapes, and asserts that result objects are stdClass before reading properties. Also tightens JiraTicketService::createTicket() to declare an object return, matching the existing is_object() guard, and drops the redundant `?? null` fallback on TestDataTrait::resolveTestDataPath() (the trait's $filepath property is non-nullable string). Removes 12 baseline entries (24 errors). Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Adds decodeJsonResponse() helper that asserts the event response is a JsonResponse with a string body and decodes it into an array<string, mixed>. Replaces the json_decode((string) $response->getContent(), true) one-liners that produced mixed values, eliminating the offsetAccess and method.nonObject errors PHPStan flagged. Switches the three logger callback expectations to self::callback() and relabels provideDefaultMessages() keys to non-numeric strings so the data provider matches its declared @return shape. Removes 10 baseline entries (27 errors). Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
- QueryCacheService::getStats() declares an explicit array shape so tests can iterate the returned tags without mixed-typed offset access. - PerformanceBenchmarkRunner coerces the values pulled out of array_column to floats with a numeric guard, fixing the array_sum mismatch. - QueryCacheServiceTest::warmUpSkipsNonCallableEntries() builds the intentionally-malformed callbacks map through a mixed-typed intermediate variable so the call site continues to type-check. Removes 4 baseline entries (8 errors). Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
… tests - Annotate the encryptToken willReturnCallback closures with `string` so the binary concat with 'encrypted_' is type-safe. - Replace `assertTrue(true)` placeholders with assertions on the entity state actually exercised by the call, or rely on existing expects()->method() expectations as the assertion source. - Restructure the throwUnauthorizedRedirect originalException test to return from the catch block so the unreachable $this->fail() after a `never`-returning call disappears. Removes 3 baseline entries (6 errors). Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
…result types - getClientWith*Mode tests: the method already returns Client, so the tail-end assertInstanceOf was always-true. Replace with a comment pointing at the existing expects() expectations as the real assertion. - Cast successful POST/GET/PUT decoded results to stdClass before reading properties so the property accesses type-check at level 10. - Annotate handlesDifferentStatusCodes() expectedExceptionClass parameter as class-string<Throwable> to satisfy expectException()'s contract. Removes the final 5 baseline entries (8 errors). Baseline now empty. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
php-cs-fixer prefers fully-imported references over fully-qualified names in @param tags. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on resolving a large number of static analysis issues by improving type safety across the codebase. Key changes include refining PHPDoc return types, implementing recursive string coercion for parsed OAuth tokens, and adding runtime type checks (such as is_scalar and is_numeric) before operations like implode and array_sum. The test suite was also refactored to include better type assertions and helper methods for decoding JSON responses. Feedback was provided regarding a change in exception testing in JiraAuthenticationServiceTest that could potentially mask regressions; a more robust assertion pattern was suggested to maintain test integrity while satisfying static analysis.
There was a problem hiding this comment.
Pull request overview
This PR removes the need for a PHPStan baseline by fixing the underlying type issues across Jira integration services and related tests, aiming to bring phpstan-baseline.neon to zero ignored errors without changing PHPStan level or exclusions.
Changes:
- Tightened production return types / phpdoc array shapes (e.g., Jira subticket APIs,
QueryCacheService::getStats(),JiraTicketService::createTicket(): object). - Refactored tests to satisfy stricter static analysis (typed callbacks, safer JSON decoding helper, stronger result type assertions).
- Reduced baseline to “empty” (but see comment re: NEON validity).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Traits/TestDataTrait.php | Removes redundant null-coalesce logic around $filepath resolution. |
| tests/Service/Integration/Jira/JiraTicketServiceTest.php | Adds typed callbacks/array-shapes and asserts returned Jira responses are stdClass. |
| tests/Service/Integration/Jira/JiraHttpClientServiceTest.php | Removes redundant always-true assertions; tightens exception-class typing via phpdoc. |
| tests/Service/Integration/Jira/JiraAuthenticationServiceTest.php | Replaces placeholder assertions and tightens callback parameter typing. |
| tests/Service/Cache/QueryCacheServiceTest.php | Adjusts callback map typing to exercise runtime guards while satisfying PHPStan. |
| tests/Performance/PerformanceBenchmarkRunner.php | Coerces benchmark metrics to numeric floats before aggregation. |
| tests/EventSubscriber/ExceptionSubscriberTest.php | Adds decodeJsonResponse() helper to assert response/body structure and reduce mixed usage. |
| src/Service/SubticketSyncService.php | Tightens documented return type for synced subticket keys. |
| src/Service/Integration/Jira/JiraTicketService.php | Makes createTicket() explicitly return object consistent with existing runtime guard. |
| src/Service/Integration/Jira/JiraOAuthApiService.php | Tightens documented return type for getSubtickets() to list<string>. |
| src/Service/Integration/Jira/JiraHttpClientService.php | Ensures error message extraction builds string lists safely from mixed JSON payloads. |
| src/Service/Integration/Jira/JiraAuthenticationService.php | Adds recursive parse_str() value stringification to avoid mixed array-to-string issues. |
| src/Service/Cache/QueryCacheService.php | Documents getStats() with an explicit array shape. |
| phpstan-baseline.neon | Baseline reduced to empty content (but currently not a valid empty ignoreErrors list). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
============================================
- Coverage 81.49% 81.47% -0.02%
- Complexity 2579 2584 +5
============================================
Files 172 172
Lines 7107 7116 +9
============================================
+ Hits 5792 5798 +6
- Misses 1315 1318 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ne yaml Two review-feedback items on PR #285: * JiraAuthenticationServiceTest::throwUnauthorizedRedirectIncludesOriginalException previously bare-returned in the catch and silently passed if the method stopped throwing. Restores `$this->fail()` after the try/catch with a scoped `@phpstan-ignore-next-line deadCode.unreachable` comment so the test detects regressions while still satisfying PHPStan's `never`-return narrowing. * phpstan-baseline.neon now uses `ignoreErrors: []` instead of an empty scalar key, which NEON parsed as `null` and could surprise downstream consumers expecting an array. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
CybotTM
added a commit
that referenced
this pull request
May 8, 2026
… rule Per gemini-code-assist review on PR #284: rather than adding 121 baseline entries (242 errors total) for the PHPUnit 13 + phpstan-phpunit stub mismatch on the legacy `->method('x')->with(...)->willReturn(...)` mock chain, switch to two targeted message-restricted ignoreErrors rules in phpstan.neon. Comment in-place documents the deprecation timeline (12.5.11 introduced, 14 removes) and that a dedicated test-modernization PR will migrate ~143 call sites before the eventual PHPUnit 14 bump. Net: phpstan-baseline.neon goes from 121 → 40 entries (242 → 80 errors). The remaining 80 are not new — they're the same set PR #285 is clearing in source; this PR will rebase against that once it merges and the baseline should converge to zero plus the two scoped rules above. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
CybotTM
added a commit
that referenced
this pull request
May 8, 2026
… rule Per gemini-code-assist review on PR #284: rather than adding 121 baseline entries (242 errors total) for the PHPUnit 13 + phpstan-phpunit stub mismatch on the legacy `->method('x')->with(...)->willReturn(...)` mock chain, switch to two targeted message-restricted ignoreErrors rules in phpstan.neon. Comment in-place documents the deprecation timeline (12.5.11 introduced, 14 removes) and that a dedicated test-modernization PR will migrate ~143 call sites before the eventual PHPUnit 14 bump. Net: phpstan-baseline.neon goes from 121 → 40 entries (242 → 80 errors). The remaining 80 are not new — they're the same set PR #285 is clearing in source; this PR will rebase against that once it merges and the baseline should converge to zero plus the two scoped rules above. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
CybotTM
added a commit
that referenced
this pull request
May 8, 2026
…rvice stdClass narrowing Resurfaced after rebasing onto main, where #284 added two ignoreErrors patterns for InvocationStubber::with()/willReturn() that this branch's mock-to-stub sweep already eliminated, and where #285's stronger Jira return narrowing made four runtime assertions tautological. - JiraHttpClientService::getOAuthConsumerKey: drop ?? '' on ticketSystem->getLogin() — the entity setter is now string-typed, so the coalesce branch is unreachable. - JiraTicketServiceTest: drop assertInstanceOf(stdClass::class, ...) in createTicket / searchTickets / getTicket / addComment tests — the service signatures already declare : stdClass. - phpstan.neon: drop the InvocationStubber::with() and willReturn() on mixed ignore patterns — every with()-on-stub call site was converted to createStub(...)->method(...)->willReturn(...) in this branch's earlier mock-to-stub sweep. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
16 tasks
CybotTM
added a commit
that referenced
this pull request
May 8, 2026
…ne cleanup, fully-green PHPUnit (#288) ## Description Comprehensive PHP modernization pass against the timetracker code base. Brings the suite to a fully-green PHPUnit 12 state, eliminates 16 entries from the PHPStan baseline by fixing the underlying type issues, adds `#[\SensitiveParameter]` to every credential-bearing method, and aligns the Rector and PHP-CS-Fixer configs with the project's actual PHP 8.5 / Symfony 8 / PER-CS targets. No production behaviour changes — every fix preserves runtime semantics; only types, mocks and tooling configs move. **Rebased onto `main` after #284 (PHPUnit 13 upgrade) and #285 (baseline cleanup) merged.** All 5 commits carry `Signed-off-by:` (DCO). ## Type of Change - [x] Code refactoring - [x] Performance improvement (test suite — fewer mock allocations, earlier PHPUnit 13 readiness) - [ ] Breaking change ## Changes Made ### Production hardening - **`#[\SensitiveParameter]` (PHP 8.2+)** — annotated 14 password / token / OAuth-secret parameters across `LdapClientService::setUserPass`, `ModernLdapService::{authenticate,validateInput}`, `JiraOAuthApiService::{getFetchAccessTokenClient,getClient,fetchOAuthAccessToken,storeToken,getOAuthAuthUrl}`, `TokenEncryptionService::{encryptToken,decryptToken,rotateToken}`, plus `TicketSystem::setPassword` and `UserTicketsystem::set{AccessToken,TokenSecret}`. Prevents secrets from leaking into stack traces / Sentry / Monolog output. - **Entity setter typing** — `TicketSystem` `set{Name,Url,Login,Password,TicketUrl,BookTime}`, `UserTicketsystem::set{AccessToken,TokenSecret}`, plus `User::$id` (`?int`). Replaces the legacy untyped Doctrine getters/setters that bypassed strict typing at call sites. - **Jira service return narrowing** — `JiraTicketService::{createTicket,searchTickets,getTicket,updateTicket,addComment}` previously declared `: mixed`/`: object`; now return `: stdClass` (the truthful runtime type from `json_decode($body, false)`). - **`JiraWorkLogPayloadDto`** (`final readonly`, namespace `App\DTO\Jira`) replaces `array $data`/`: mixed` in `JiraWorkLogService::{prepareWorkLogData,createWorkLog,updateWorkLog}` with a typed payload + serializer; removes the now-dead `is_object()` guard in `updateEntryWorkLog`. - **`OptimizedEntryRepository::findByFilterArrayOptimized`** — promoted `array $filter` to a precise PHPDoc array-shape (`{user_id?: int, customer_id?: int, ...}`) so PHPStan can verify each downstream `ArrayTypeHelper::getInt` call. - **Two genuine bugs unmasked by tightened types** — removed dead `null === $ticketUrl` branch in `Entry::getTicketSystemIssueLink` and dead `?? ''` on `JiraHttpClientService::getOAuthConsumerKey` (the entity setter is now string-typed, so the coalesce branch is unreachable). - **Deprecation cleanup** — `ExportService::getEntries{Ticket,Worklog}Url` no longer uses null as an array-offset (cache key now `getId() ?? ''`); `TimeCalculationService::formatDuration` swaps an implicit float `%` int for `(int) floor(fmod(...))`. ### PHPUnit 12/13 cleanup | Before | After | |---|---| | 1 error | **0** | | 14 warnings | **0** | | 6 deprecations | **0** | | 495 notices (1698 occurrences) | **0** | | 10 risky | **0** | - **`createMock` → `createStub`** for 1698 sites across 29 test files where no expectations are set (PHPUnit 12 distinguishes mocks from stubs). 19 classes received `#[AllowMockObjectsWithoutExpectations]` at class level instead, where the same fixture is reused with mixed semantics across many tests. - **Stripped 67 deprecated `->with(...)`** calls from stub-style chains (deprecated in 12, hard error in 14). Removes the two `InvocationStubber::with()` / `willReturn() on mixed` ignore patterns from `phpstan.neon` introduced by #284 — every `with()`-on-stub call site is now properly migrated. - Converted `$this->createStub(...)` → `self::createStub(...)` to satisfy PHPStan's static-call requirement. - **Performance test cleanup** — replaced naive `unlink + rmdir` with `Symfony\Filesystem::remove()`; clears 14 `Is a directory` / `Directory not empty` warnings. - **Exception-handler restoration** — added `tearDown { restore_exception_handler(); parent::tearDown(); }` to `EntryRepositoryExportTest`, balancing Symfony's `bootKernel()` registration; clears 4 risky markers. - **Real assertion** — `EntryRepositoryFullIntegrationTest::testFindOverlappingEntriesExcludesSpecifiedId` now uses a single `assertNotContains` instead of an empty-result-tolerant foreach. - **JiraOAuthApiService early-return tests** — dropped the `expectNotToPerformAssertions()` calls; the existing `mock->method(...)` calls are themselves the verifications PHPUnit was counting. - Deleted redundant `EntryTest::testGetTicketSystemIssueLinkReturnsTicketWhenNullTicketUrl` (sibling empty-string test already covers the same intent under the now-non-nullable `getTicketUrl(): string` contract). - Removed 4 tautological `assertInstanceOf(stdClass::class, $result)` calls in `JiraTicketServiceTest` since the service now declares `: stdClass`. ### Tooling alignment - **Rector** — bumped `LevelSetList::UP_TO_PHP_84` → `UP_TO_PHP_85`, dropped `SymfonySetList::SYMFONY_73` (per-version sets are `@deprecated` upstream) in favour of `->withComposerBased(symfony: true)` which auto-detects the installed Symfony 8 line, and added `->withAttributesSets(symfony: true)` so future annotation→attribute migrations run automatically. Applied the one finding the bump surfaced (`array_values($x)[0]` → `array_first($x)` in `SaveContractAction.php`). - **PHP-CS-Fixer** — bumped `@PHP84Migration` → `@PHP85Migration`. Codebase was already 8.5-clean: zero diff from the bump itself. - **Stale config cleanup** — deleted 4 unreferenced files (root `rector.php` placeholder, `config/quality/{phpstan.dist.neon,phpstan-baseline.neon,phpstan-phpat.neon}`) plus 90 orphaned PHPStan cache files under `config/quality/var/` (already in `.gitignore` line 62, so they shouldn't have been tracked). Verified no hits in CI/Makefile/composer for the deleted files. ## Testing - [x] **`make test`** (Docker) — `Tests: 1922, Assertions: 5501, Skipped: 1`, 0 errors / 0 failures / 0 warnings / 0 deprecations / 0 notices / 0 risky (the 1 skip is intentional and pre-existing). - [x] **`composer check:all`** — PHPStan level 10 + bleedingEdge `[OK] No errors` (311 files); PHPat `[OK] No errors`; PHP-CS-Fixer clean; Twig lint clean. - [x] **`bin/rector … --dry-run`** — clean. - [x] **`composer security-check`** — no advisories. - [x] **`npm run build`** — webpack compiled successfully, no warnings, 2106 assets emitted. - [x] **`make e2e`** (Playwright on ephemeral compose stack) — **118 passed, 1 failed, 2 skipped**. The single failing test (`settings.spec.ts:142 › should save show_empty_line setting`) is a parallel-state flake: it passes 3/3 in isolation; the failure is a race when the full suite runs concurrently. Not a regression caused by this branch. - [x] Manual testing completed. ## Code Quality - [x] Code follows project coding standards (`@PER-CS:risky` + `@PHP85Migration`) - [x] Self-review completed - [ ] Documentation updated — none needed; all changes are internal types and tooling - [x] No breaking changes — public API is unchanged; the `: stdClass` narrowing on `JiraTicketService` is a covariant tightening (callers already accessed `$result->key`/`$result->total` style on `stdClass`) ## Migration Notes - **`composer rector` cannot be safely tested via `composer rector -- --dry-run`** — composer doesn't forward flags through that script, so the dry-run flag is silently dropped and rector runs in apply-mode. Use `bin/rector process src --config=config/quality/rector.php --dry-run` directly when previewing. - **Tests must run inside the Docker `app-dev` container** (per the existing `make test` target). The host typically lacks `pdo_mysql` and the `db_unittest` hostname only resolves on the compose network. - **All 5 commits carry `Signed-off-by:` (DCO)**. The branch was rebased onto `main` after #284 and #285 merged. ## Checklist - [x] I have read the [CONTRIBUTING](https://github.com/netresearch/timetracker/blob/main/CONTRIBUTING.md) guidelines - [x] I agree to follow the [Code of Conduct](https://github.com/netresearch/timetracker/blob/main/CODE_OF_CONDUCT.md)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Drives
phpstan-baseline.neonfrom 80 errors / 40 entries down to 0 by fixing the underlying issues — no new ignore patterns, no level changes, no path exclusions.Patterns fixed
implode/natcasesortoverlist<mixed>Service/Integration/Jira/JiraAuthenticationService,JiraHttpClientService,JiraOAuthApiService,Service/SubticketSyncService,Command/TtSyncSubticketsCommandgetSubtickets/syncProjectSubticketstolist<string>; recursivestringifyParsedValue()helper forparse_stroutput;array_map(strval-equivalent)over decoded JSON arrays.nullCoalesce.propertyon non-nullable$filepathTraits/TestDataTrait?? null.mixed(offsetAccess, method.nonObject, str_contains arg)EventSubscriber/ExceptionSubscriberTest,Service/Integration/Jira/JiraTicketServiceTestdecodeJsonResponse()helper that assertsJsonResponse+stringbody and returnsarray<string, mixed>; in-line array shapes for callback payloads.staticMethod.dynamicCallforAssert::callback()ExceptionSubscriberTest,JiraTicketServiceTestself::callback().data-provider return type mismatchExceptionSubscriberTest::provideDefaultMessagesarray<string, …>shape.array_sumoverlist<mixed>Performance/PerformanceBenchmarkRunnerarray_column()output tofloatwith a numeric guard.warmUp()argument type andassertContainsagainst mixedService/Cache/QueryCacheServiceTest+ productionQueryCacheService::getStats()getStats()to an explicit array shape; built the deliberately-malformed callbacks map through a mixed intermediate so the call site type-checks.binaryOp.invalidfor'encrypted_' . $tokenJiraAuthenticationServiceTestwillReturnCallbacklambdas withstring $token.assertTrue(true)placeholders, unreachable$this->fail()afterneverJiraAuthenticationServiceTestnever-return test toreturnfrom the catch.assertInstanceOf(Client::class, $client)always-true, mixed property readsJiraHttpClientServiceTeststdClassbefore reading properties; annotated the data-providerexpectedExceptionClassasclass-string<Throwable>.JiraTicketService::createTicket(): objectis_object()guard.Numbers
Test plan
phpstan analyze --no-progress→ No errorsphpstan analyze --generate-baseline→ would emit empty baselinemake test→ identical pre-existing failures only (LDAP integration tests need the dev profile up;EntryRepositoryExportTestexception-handler issue;ExportActionPerformanceTestflake noted in the task brief). Verified by re-running onmainwith my changes stashed.php-cs-fixer fix --dry-run --diff→ clean after applying the one suggestion (Throwable import).