refactor(php): modernization pass — types, SensitiveParameter, baseline cleanup, fully-green PHPUnit#288
Merged
Merged
Conversation
![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 updates the project to PHP 8.5 standards and significantly improves type safety and security across the codebase. Key changes include the application of the #[SensitiveParameter] attribute to sensitive data, the introduction of a JiraWorkLogPayloadDto, and the refinement of return types in Jira-related services. Feedback highlights opportunities to reduce code duplication by sharing stringification logic and to improve consistency by using stdClass return types instead of generic objects.
There was a problem hiding this comment.
Pull request overview
Modernization/refactor pass across the Symfony/PHP codebase to improve type safety, prevent secret leakage in stack traces (via #[SensitiveParameter]), and bring PHPUnit 12 + PHPStan baseline/tooling to a clean, green state.
Changes:
- Tightened types across entities/services (incl. Jira service return narrowing) and added
#[SensitiveParameter]annotations to credential/token parameters. - Refactored Jira worklog handling to use a typed payload DTO instead of unstructured arrays.
- Updated tests for PHPUnit 12 (mock vs stub usage, risky test cleanup) and aligned quality tooling configs/baselines; removed committed PHPStan cache artifacts.
Reviewed changes
Copilot reviewed 115 out of 145 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Validator/UniqueUsernameValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/UniqueUserAbbrValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/UniqueTicketSystemNameValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/UniqueTeamNameValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/UniqueProjectNameForCustomerValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/UniqueCustomerNameValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/UniqueActivityNameValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/Constraints/UniqueTeamNameValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Validator/Constraints/CustomerTeamsRequiredValidatorTest.php | PHPUnit 12 mock/stub cleanup and allow-mocks-without-expectations attribute. |
| tests/Util/RequestEntityHelperTest.php | Refactors registry/repository doubles toward stubs. |
| tests/Traits/TestDataTrait.php | Removes dead null-coalesce and simplifies empty-path handling for fixture resolution. |
| tests/Service/Security/TokenEncryptionServiceTest.php | Switches ParameterBag double to a stub where no expectations are asserted. |
| tests/Service/Ldap/ModernLdapServiceTest.php | Moves some dependencies to stubs and adds allow-mocks-without-expectations attribute. |
| tests/Service/Integration/Jira/JiraTicketServiceTest.php | Adds allow-mocks-without-expectations attribute for PHPUnit 12. |
| tests/Service/Integration/Jira/JiraOAuthApiFactoryTest.php | Converts simple collaborators to stubs. |
| tests/Service/Integration/Jira/JiraHttpClientServiceTest.php | Converts some Guzzle clients to stubs; removes deprecated stub-chaining patterns. |
| tests/Service/ExportServiceTest.php | Converts repositories/registry/router doubles to stubs for PHPUnit 12. |
| tests/Service/Cache/QueryCacheServiceTest.php | Converts CacheItem doubles to stubs and adds allow-mocks-without-expectations attribute. |
| tests/Security/LdapAuthenticatorTest.php | Converts several dependencies to stubs; adds allow-mocks-without-expectations attribute. |
| tests/Repository/EntryRepositoryFullIntegrationTest.php | Ensures a real assertion runs when result sets are empty. |
| tests/Repository/EntryRepositoryExportTest.php | Restores exception handler in tearDown to avoid PHPUnit risky-test flags. |
| tests/Performance/ExportActionPerformanceTest.php | Uses Symfony Filesystem for recursive temp-dir cleanup. |
| tests/EventSubscriber/ExceptionSubscriberTest.php | Adds typed JSON decode helper and updates callback usage for static analysis. |
| tests/Entity/UserTest.php | Converts TicketSystem double to stub. |
| tests/Entity/EntryTest.php | Converts several doubles to stubs and removes now-dead null-ticketUrl test. |
| src/Service/Util/TimeCalculationService.php | Fixes minutes calculation for float durations using fmod() + floor(). |
| src/Service/SubticketSyncService.php | Narrows subticket list to strings and filters non-scalar values before sorting. |
| src/Service/Security/TokenEncryptionService.php | Adds #[SensitiveParameter] to token-bearing methods. |
| src/Service/Ldap/ModernLdapService.php | Adds #[SensitiveParameter] to password-bearing methods. |
| src/Service/Ldap/LdapClientService.php | Adds #[SensitiveParameter] to password setter. |
| src/Service/Integration/Jira/JiraWorkLogService.php | Replaces array payloads with a typed worklog payload DTO and narrows response types. |
| src/Service/Integration/Jira/JiraTicketService.php | Narrows several API methods to stdClass and centralizes response validation. |
| src/Service/Integration/Jira/JiraOAuthApiService.php | Adds #[SensitiveParameter] to OAuth token parameters. |
| src/Service/Integration/Jira/JiraHttpClientService.php | Adds #[SensitiveParameter], makes error message stringification safe for mixed arrays, and removes dead null-coalesce. |
| src/Service/Integration/Jira/JiraAuthenticationService.php | Makes token extraction robust by safely stringifying mixed token values. |
| src/Service/ExportService.php | Avoids null array offsets by using a safe cache key for ticket systems without IDs. |
| src/Repository/OptimizedEntryRepository.php | Adds precise array-shape PHPDoc for filter typing and removes redundant casts. |
| src/Entity/UserTicketsystem.php | Types token setters and annotates sensitive parameters. |
| src/Entity/User.php | Types the Doctrine ID as ?int to reflect pre-persist state. |
| src/Entity/TicketSystem.php | Adds typed setters/getters and marks password parameter as sensitive. |
| src/Entity/Entry.php | Removes dead null-branch now that ticket URL is non-nullable. |
| src/DTO/Jira/JiraWorkLogPayloadDto.php | Introduces a readonly DTO for Jira worklog payload serialization. |
| src/Controller/Admin/SaveContractAction.php | Uses a “first element” helper in contract update flow. |
| rector.php | Removes unused root-level Rector config placeholder. |
| phpstan-baseline.neon | Shrinks baseline by removing resolved ignores. |
| config/quality/var/phpstan/cache/PHPStan/f2/dc/f2dc2d17997eb538e5ee9671aae540706e6b7e1f.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/de/9b/de9b38edb11890e6f7d7c1423355594d01d1325b.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/da/5a/da5ac1892a4737cb5da031f1ba7145994641dee3.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/cf/2c/cf2c06a4a7dd7de930aca2841b7913929f318d29.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/cache-cleared | Removes committed PHPStan cache marker. |
| config/quality/var/phpstan/cache/PHPStan/bf/70/bf70592e25fffaa76a5c905d896eb8a9d45e1794.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/9e/d7/9ed741fa1937e8e6793629f75c0e3eec84a0fb25.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/92/22/922221eaeb72919ad4e52fd3837bcbfbae6e5472.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/91/cb/91cb710cfcf3a66c23c93e7d7740494ba26f895a.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/90/20/90202f7fb1d0cc7c45cc94190cc0b9f06d51c818.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/86/1c/861c871c260094021c41d6ea301d7348705a9d4e.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/63/c2/63c2455c5eb33219eef527a520d833a9f7066929.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/62/71/6271531ea3dfcade7c96d4e29139303e5a0e43eb.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/5c/62/5c622351b90aac689c891c84f06eee1b4b6e1598.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/4c/99/4c9993f94026e4c8fefb90e07689530d5c9b23ae.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/48/1a/481a649e696d30cb621e5e6803a5abdbe724da88.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/44/93/44930e96a068c335fbb03f903e1719c5904587e6.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/42/78/4278300ba2c1b9416fb7644ff09e1feba32fc884.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/27/1c/271c74e18b1b907cab45edae620e6667fa9190c9.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/24/bd/24bdcab1ce78da7c6439c0f318784396fbeb5819.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/21/13/2113105846fefd104a7a515718d13bba0ee68d5d.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/10/b4/10b477db680d1c26dafd647e2176bc84dfe3e2c3.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/0a/5f/0a5f913079dc6f704c4a8121a3d99a42b6462fc9.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/PHPStan/08/1d/081d01c99bb2f154618b9f4c79c767f677e75861.php | Removes committed PHPStan cache artifact. |
| config/quality/var/phpstan/cache/nette.configurator/container.journal | Removes committed PHPStan DI container journal. |
| config/quality/var/phpstan/cache/nette.configurator/Container_feb3a04636.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_ef3bef3da3.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_ed65fa3866.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_e492b1e17f.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_d0e1967d04.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_a389e622c4.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_7268add194.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan/cache/nette.configurator/Container_184ddc6448.php.lock | Removes committed PHPStan DI container lock file. |
| config/quality/var/phpstan-phpat/cache/PHPStan/f2/dc/f2dc2d17997eb538e5ee9671aae540706e6b7e1f.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/de/9b/de9b38edb11890e6f7d7c1423355594d01d1325b.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/da/5a/da5ac1892a4737cb5da031f1ba7145994641dee3.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/cf/2c/cf2c06a4a7dd7de930aca2841b7913929f318d29.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/cache-cleared | Removes committed PHPat PHPStan cache marker. |
| config/quality/var/phpstan-phpat/cache/PHPStan/bf/70/bf70592e25fffaa76a5c905d896eb8a9d45e1794.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/9e/d7/9ed741fa1937e8e6793629f75c0e3eec84a0fb25.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/92/22/922221eaeb72919ad4e52fd3837bcbfbae6e5472.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/91/cb/91cb710cfcf3a66c23c93e7d7740494ba26f895a.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/90/20/90202f7fb1d0cc7c45cc94190cc0b9f06d51c818.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/86/1c/861c871c260094021c41d6ea301d7348705a9d4e.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/63/c2/63c2455c5eb33219eef527a520d833a9f7066929.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/62/71/6271531ea3dfcade7c96d4e29139303e5a0e43eb.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/5c/62/5c622351b90aac689c891c84f06eee1b4b6e1598.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/4c/99/4c9993f94026e4c8fefb90e07689530d5c9b23ae.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/48/1a/481a649e696d30cb621e5e6803a5abdbe724da88.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/44/93/44930e96a068c335fbb03f903e1719c5904587e6.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/42/78/4278300ba2c1b9416fb7644ff09e1feba32fc884.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/27/1c/271c74e18b1b907cab45edae620e6667fa9190c9.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/24/bd/24bdcab1ce78da7c6439c0f318784396fbeb5819.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/21/13/2113105846fefd104a7a515718d13bba0ee68d5d.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/0a/5f/0a5f913079dc6f704c4a8121a3d99a42b6462fc9.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/PHPStan/08/1d/081d01c99bb2f154618b9f4c79c767f677e75861.php | Removes committed PHPat PHPStan cache artifact. |
| config/quality/var/phpstan-phpat/cache/nette.configurator/container.journal | Removes committed PHPat DI container journal. |
| config/quality/var/phpstan-phpat/cache/nette.configurator/Container_2a9e3ae780.php.lock | Removes committed PHPat DI container lock file. |
| config/quality/rector.php | Updates Rector sets to PHP 8.5 and composer-based Symfony detection/attribute sets. |
| config/quality/phpstan.dist.neon | Removes unused legacy PHPStan config file. |
| config/quality/phpstan-phpat.neon | Removes unused legacy PHPat PHPStan config file. |
| .php-cs-fixer.dist.php | Updates fixer preset to PHP 8.5 migration set and clarifies risky-set availability. |
Comments suppressed due to low confidence (1)
tests/Util/RequestEntityHelperTest.php:105
- Switching
ManagerRegistryto a stub removes verification thatgetRepository()is called with the expected entity class. This weakens the test’s ability to catch regressions inRequestEntityHelperargument wiring; keepManagerRegistryas a mock (and assertwith(User::class)/with(TicketSystem::class)as appropriate).
… baseline - Add #[\SensitiveParameter] to 14 password/token/secret params across Ldap*, Jira*, TokenEncryption services and TicketSystem/UserTicketsystem entities, preventing credential leakage into stack traces and Sentry/Monolog output. - Type 6 legacy untyped Doctrine setters in TicketSystem (setName/Url/Login/ Password/TicketUrl/BookTime) and 2 in UserTicketsystem (setAccessToken/ TokenSecret) with native PHP types; type User::$id as ?int. - Narrow the 5 ': mixed' returns in JiraTicketService to ': stdClass' to match the actual json_decode(..., false) contract; introduce ensureObjectResponse() helper that throws JiraApiException on malformed responses. - Introduce App\DTO\Jira\JiraWorkLogPayloadDto (final readonly) replacing ': mixed' arrays in JiraWorkLogService::create/updateWorkLog and prepareWorkLogData; remove now-dead is_object() guard. - Tighten PHPDoc generics: array-shape on OptimizedEntryRepository::findByFilterArrayOptimized; list<string> for Jira field-name params. - Fix and prune 16 entries (-114 lines / -47%) from phpstan-baseline.neon by resolving the underlying type-narrowing issues: array<string> implode coercion in JiraAuthenticationService, JiraHttpClientService, SubticketSyncService, TtSyncSubticketsCommand; mixed offset access and Response|null guards in ExceptionSubscriberTest; dead null-coalesce in TestDataTrait. - Remove dead null checks exposed by entity type tightening (Entry::getTicketUrl path, JiraHttpClientService::getOAuthConsumerKey). - Delete 4 stale duplicate config files unreferenced by Makefile/CI/composer: 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/. PHPStan level 10 + bleedingEdge passes clean (311 files, treatPhpDocTypesAsCertain: false). Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Brings the suite to a fully-green PHPUnit 12 state:
before: 1 error, 14 warnings, 6 deprecations, 495 notices, 10 risky
after: 0/0/0/0/0 (1 intentional skip, 1922 tests, 5470 assertions)
Production fixes
- ExportService::getEntries{Ticket,Worklog}Url: extract nullable
TicketSystem id to a local that defaults to '' for the per-system
cache key, removing 5x 'null as array offset' deprecations.
- TimeCalculationService::formatDuration: replace implicit float % int
with float-safe '(int) floor(fmod(...))' to remove 'Implicit
float-to-int loses precision' deprecation.
- JiraAuthenticationService / JiraHttpClientService: convert two
locally-called static helpers to instance methods (Rector
LocallyCalledStaticMethodToNonStaticRector).
Test fixes
- Convert createMock -> createStub on 1698 sites across 29 test files
where no expectations are set (PHPUnit 12 distinguishes mocks from
stubs). Apply class-level #[AllowMockObjectsWithoutExpectations] in
19 classes where the same fixture is reused with mixed semantics.
- Strip 67 deprecated ->with(...) calls from stub-style chains
(deprecated in 12, hard error in 13).
- Convert $this->createStub(...) -> self::createStub(...) per PHPStan's
static-call requirement on inherited static methods.
- ExportActionPerformanceTest::tearDown: replace naive
unlink+rmdir with Symfony Filesystem::remove for recursive,
idempotent cleanup; clears 14 'Is a directory' / 'Directory not
empty' warnings.
- EntryRepositoryExportTest: add tearDown calling
restore_exception_handler() to balance Symfony's bootKernel handler
registration; clears 4 'did not remove its own exception handlers'
risky markers.
- EntryRepositoryFullIntegrationTest::testFindOverlappingEntriesExcludesSpecifiedId:
replace empty-result-tolerant foreach loop with a single
assertNotContains; clears 'no assertions' risky.
- JiraOAuthApiServiceTest: drop expectNotToPerformAssertions in 5
early-return tests where mock ->method(...) calls already function
as the verification; clears 'expectsNoAssertions but performed N'
risky markers.
- EntryTest::testGetTicketSystemIssueLinkReturnsTicketWhenNullTicketUrl:
delete; sibling testGetTicketSystemIssueLinkReturnsTicketWhenEmptyTicketUrl
already covers the same intent under the now-non-nullable
TicketSystem::getTicketUrl(): string contract.
Tooling state
- PHPStan level 10 + bleedingEdge: [OK] No errors (311 files)
- PHPat architecture: [OK] No errors
- PHP-CS-Fixer @PER-CS:risky + @PHP84Migration: clean
- Twig lint: clean
- Local PHP Security Checker: no advisories
- Rector dry-run: clean
- npm run build: compiled successfully, no warnings
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
The Rector config was on UP_TO_PHP_84 + SymfonySetList::SYMFONY_73
while the project requires PHP ^8.5 and Symfony ^8.0.
- LevelSetList::UP_TO_PHP_84 -> UP_TO_PHP_85: enables the
PHP 8.5 set (e.g. ArrayFirstLastRector for array_values()[0]
-> array_first()).
- Drop SymfonySetList::SYMFONY_73 (per-version sets are now
@deprecated upstream) in favour of ->withComposerBased(
symfony: true) which auto-detects the installed Symfony 8 line
and applies the matching rules.
- Add ->withAttributesSets(symfony: true) so future annotation->
attribute migrations run automatically.
- Keep SymfonySetList::SYMFONY_CODE_QUALITY (still non-deprecated).
Apply the one finding surfaced by UP_TO_PHP_85:
src/Controller/Admin/SaveContractAction.php:155
array_values($contractsOld)[0] -> array_first($contractsOld)
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
PHP-CS-Fixer 3.95.1 ships @PHP85Migration; the project requires PHP ^8.5, so no reason to lag a release behind. The config comment about @PHP83Migration:risky / @PHP84Migration:risky still applies (also no @PHP85Migration:risky exists yet), so the risky preset remains @PHP82Migration:risky. Codebase is already PHP 8.5-clean: enabling the new set produces zero diff. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
…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>
CybotTM
force-pushed
the
feat/php-modernization-pass
branch
from
74b92d2 to
aefadd1
Compare
Member
Author
|
/gemini review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
=========================================
Coverage 81.41% 81.41%
- Complexity 2584 2588 +4
=========================================
Files 172 173 +1
Lines 7085 7107 +22
=========================================
+ Hits 5768 5786 +18
- Misses 1317 1321 +4
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:
|
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by bumping PHP migration levels to 8.5, enhancing type safety with explicit hints and return types, and improving security through the use of the #[SensitiveParameter] attribute. It also refactors the test suite to utilize modern PHPUnit features like stubs and attributes. A high-severity issue was identified in SubticketSyncService where a scalar check on Jira sub-task objects incorrectly prevents subtickets from being collected; a suggestion was provided to correctly extract the ticket keys from the returned objects.
…l at file level
Two fixes for the test infrastructure surfaced by the modernization
pass:
- Makefile: e2e-down used `COMPOSE_PROFILES=e2e docker compose
down`, which tears down all containers that were started under the
e2e profile, including ldap-dev pulled in transitively via
`app-e2e`'s `depends_on`. ldap-dev is a shared dev/development/test
fixture though, so a subsequent `make test` (LDAP integration
suite) errors out with `Can't contact LDAP server`. Scope the
teardown to the e2e-only services (httpd-e2e, app-e2e, db-e2e)
via an explicit list and `compose stop && compose rm -fs`,
leaving ldap-dev running.
- e2e/settings.spec.ts: only the first describe (`Settings Tab`)
declared `mode: 'serial'`. The other two (`Settings Effectiveness`,
`Settings API`) ran in parallel against the same `i.myself` user,
racing with `Settings Tab` and producing a flaky failure on
`should save show_empty_line setting`. Lift the serial config to
file level so all three describes run sequentially. Verified
stable across two consecutive 8-test runs.
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
…n real mocks Per PR #288 review (Copilot), the earlier mock->stub sweep over-stripped ->with(...) from JiraHttpClientServiceTest and RequestEntityHelperTest, leaving real mocks (with expects()) without their argument-contract assertions. Restore them on every flagged site, plus apply the same upgrade to the 7 sibling tests in RequestEntityHelperTest so all find()/getRepository() calls verify the ID and entity-class are wired through correctly. Args verified against the actual production code paths in JiraHttpClientService and RequestEntityHelper: - getTokens($user, $ticketSystem) - throwUnauthorizedRedirect($ticketSystem) - Client::request(METHOD, /rest/api/latest/...PATH, options) where options always carries auth=oauth and adds json=$data on POST/PUT but not DELETE - getRepository(<entity-class>) and find(<id>) Tests upgraded from createStub to createMock where verification was needed; #[AllowMockObjectsWithoutExpectations] retained on the class for genuinely-stub fixtures. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
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.
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
mainafter #284 (PHPUnit 13 upgrade) and #285 (baseline cleanup) merged. All 5 commits carrySigned-off-by:(DCO).Type of Change
Changes Made
Production hardening
#[\SensitiveParameter](PHP 8.2+) — annotated 14 password / token / OAuth-secret parameters acrossLdapClientService::setUserPass,ModernLdapService::{authenticate,validateInput},JiraOAuthApiService::{getFetchAccessTokenClient,getClient,fetchOAuthAccessToken,storeToken,getOAuthAuthUrl},TokenEncryptionService::{encryptToken,decryptToken,rotateToken}, plusTicketSystem::setPasswordandUserTicketsystem::set{AccessToken,TokenSecret}. Prevents secrets from leaking into stack traces / Sentry / Monolog output.TicketSystemset{Name,Url,Login,Password,TicketUrl,BookTime},UserTicketsystem::set{AccessToken,TokenSecret}, plusUser::$id(?int). Replaces the legacy untyped Doctrine getters/setters that bypassed strict typing at call sites.JiraTicketService::{createTicket,searchTickets,getTicket,updateTicket,addComment}previously declared: mixed/: object; now return: stdClass(the truthful runtime type fromjson_decode($body, false)).JiraWorkLogPayloadDto(final readonly, namespaceApp\DTO\Jira) replacesarray $data/: mixedinJiraWorkLogService::{prepareWorkLogData,createWorkLog,updateWorkLog}with a typed payload + serializer; removes the now-deadis_object()guard inupdateEntryWorkLog.OptimizedEntryRepository::findByFilterArrayOptimized— promotedarray $filterto a precise PHPDoc array-shape ({user_id?: int, customer_id?: int, ...}) so PHPStan can verify each downstreamArrayTypeHelper::getIntcall.null === $ticketUrlbranch inEntry::getTicketSystemIssueLinkand dead?? ''onJiraHttpClientService::getOAuthConsumerKey(the entity setter is now string-typed, so the coalesce branch is unreachable).ExportService::getEntries{Ticket,Worklog}Urlno longer uses null as an array-offset (cache key nowgetId() ?? '');TimeCalculationService::formatDurationswaps an implicit float%int for(int) floor(fmod(...)).PHPUnit 12/13 cleanup
createMock→createStubfor 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.->with(...)calls from stub-style chains (deprecated in 12, hard error in 14). Removes the twoInvocationStubber::with()/willReturn() on mixedignore patterns fromphpstan.neonintroduced by chore(deps): upgrade phpunit 12 -> 13 #284 — everywith()-on-stub call site is now properly migrated.$this->createStub(...)→self::createStub(...)to satisfy PHPStan's static-call requirement.unlink + rmdirwithSymfony\Filesystem::remove(); clears 14Is a directory/Directory not emptywarnings.tearDown { restore_exception_handler(); parent::tearDown(); }toEntryRepositoryExportTest, balancing Symfony'sbootKernel()registration; clears 4 risky markers.EntryRepositoryFullIntegrationTest::testFindOverlappingEntriesExcludesSpecifiedIdnow uses a singleassertNotContainsinstead of an empty-result-tolerant foreach.expectNotToPerformAssertions()calls; the existingmock->method(...)calls are themselves the verifications PHPUnit was counting.EntryTest::testGetTicketSystemIssueLinkReturnsTicketWhenNullTicketUrl(sibling empty-string test already covers the same intent under the now-non-nullablegetTicketUrl(): stringcontract).assertInstanceOf(stdClass::class, $result)calls inJiraTicketServiceTestsince the service now declares: stdClass.Tooling alignment
LevelSetList::UP_TO_PHP_84→UP_TO_PHP_85, droppedSymfonySetList::SYMFONY_73(per-version sets are@deprecatedupstream) 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)inSaveContractAction.php).@PHP84Migration→@PHP85Migration. Codebase was already 8.5-clean: zero diff from the bump itself.rector.phpplaceholder,config/quality/{phpstan.dist.neon,phpstan-baseline.neon,phpstan-phpat.neon}) plus 90 orphaned PHPStan cache files underconfig/quality/var/(already in.gitignoreline 62, so they shouldn't have been tracked). Verified no hits in CI/Makefile/composer for the deleted files.Testing
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).composer check:all— PHPStan level 10 + bleedingEdge[OK] No errors(311 files); PHPat[OK] No errors; PHP-CS-Fixer clean; Twig lint clean.bin/rector … --dry-run— clean.composer security-check— no advisories.npm run build— webpack compiled successfully, no warnings, 2106 assets emitted.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.Code Quality
@PER-CS:risky+@PHP85Migration): stdClassnarrowing onJiraTicketServiceis a covariant tightening (callers already accessed$result->key/$result->totalstyle onstdClass)Migration Notes
composer rectorcannot be safely tested viacomposer 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. Usebin/rector process src --config=config/quality/rector.php --dry-rundirectly when previewing.app-devcontainer (per the existingmake testtarget). The host typically lackspdo_mysqland thedb_unittesthostname only resolves on the compose network.Signed-off-by:(DCO). The branch was rebased ontomainafter chore(deps): upgrade phpunit 12 -> 13 #284 and chore(quality): clear phpstan baseline #285 merged.Checklist