Cleanup phpstan baseline issues by tuj · Pull Request #308 · itk-dev/economics

tuj · 2026-05-17T09:53:43Z

Link to ticket

https://leantime.itkdev.dk/TimeTable/#/tickets/showTicket/7273

Description

Cleanup phpstan baseline issues.

PHPStan baseline: 530 → 0.
Migration Version20260517131038: 18 columns made nullable on synced entities
(issue, project, version, worklog) plus 3 DateTime fields, to match property types.
Migration Version20260517151632: 11 NOT NULL ManyToOne FKs relaxed to NULL;
application-layer #[Assert\NotNull] / form validators still enforce required-ness.
Replaced ?T $prop = null with non-null defaults on 25 entity properties to satisfy
NOT NULL columns; Worker/WorkerGroup __toString() switched from ?? to ?:.
ForecastReportService rendered empty epic tags — called non-existent Epic::getName();
now uses Epic::getTitle().
SubscriptionHandlerService::getVersion() looked up Version by non-existent
versionId field; now uses find($versionId).
DataProviderService::setTimeSpentSeconds() silently lost precision passing float to an
int setter; explicit (int) cast added.
Report services (ForecastReportService, BillableUnbilledHoursReportService,
InvoicingRateReportService) now skip worklogs with null project/issue instead of crashing.
SubscriptionController::check() returns HTTP 400 on null User::getEmail();
User::getUserIdentifier() throws on empty email (Symfony requires non-empty-string).
LeantimeApiService switched from json_decode($json, null) to json_decode($json, true);
all $data->property accesses converted to array access. Wire shape unchanged.
Removed dead code: readonly string $id on 3 report-data models, unreachable break; after
throw, null check on Row iterator, unused $paginator arg in SubscriptionRepository.
Fixed Epic::getIssues() PHPDoc typo (Collection<int, Epic> → Collection<int, Issue>).

Checklist

My code is covered by test cases.
My code passes our test (all our tests).
My code passes our static analysis suite.
My code passes our continuous integration process.

…s across 9 entity files

…type surface had been hiding

…lability

…match the property types they bind to.

… issue rather than dereferencing nullable getters

codecov-commenter · 2026-05-17T19:59:58Z

Codecov Report

❌ Patch coverage is 31.48148% with 111 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feature/psalm-to-phpstan@b3b0d9f). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/Service/InvoicingRateReportService.php	0.00%	16 Missing ⚠️
src/Service/ForecastReportService.php	0.00%	14 Missing ⚠️
src/Service/LeantimeApiService.php	76.78%	13 Missing ⚠️
src/Controller/SubscriptionController.php	0.00%	9 Missing ⚠️
src/Controller/ProjectBillingController.php	0.00%	8 Missing ⚠️
src/Service/BillableUnbilledHoursReportService.php	0.00%	8 Missing ⚠️
src/Controller/InvoiceController.php	0.00%	7 Missing ⚠️
src/Controller/ManagementReportController.php	0.00%	7 Missing ⚠️
src/Service/HourReportService.php	0.00%	4 Missing ⚠️
src/DataFixtures/AppFixtures.php	0.00%	3 Missing ⚠️
... and 13 more

Additional details and impacted files

@@                     Coverage Diff                     @@
##             feature/psalm-to-phpstan     #308   +/-   ##
===========================================================
  Coverage                            ?   17.44%           
  Complexity                          ?     1714           
===========================================================
  Files                               ?      202           
  Lines                               ?     7155           
  Branches                            ?        0           
===========================================================
  Hits                                ?     1248           
  Misses                              ?     5907           
  Partials                            ?        0

Flag	Coverage Δ
	`17.44% <31.48%> (?)`
unittests	`17.44% <31.48%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jeppekroghitk

A few comments to consider before merging. Nice job.

jeppekroghitk · 2026-06-02T06:56:53Z

+{
+    public function getDescription(): string
+    {
+        return 'Make columns nullable on synced entities (issue/project/version/worklog) and on app-managed DateTime fields (project_billing period_start/end, service_agreement valid_from) to match PHP property types.';


I don't like the idea that Claude edited this file. Description is not filled out when doctrine generates it.

Claude only wrote the descriptions.

jeppekroghitk · 2026-06-02T06:57:02Z

+{
+    public function getDescription(): string
+    {
+        return 'Make ManyToOne foreign keys nullable on entities whose properties are typed nullable. Aligns DB schema with PHP property types; application/validation layer still enforces required-ness on app-managed entities.';


Same as above.

Claude only wrote the descriptions.

jeppekroghitk · 2026-06-02T07:01:47Z

-                if (null === $row) {
-                    continue;
-                }


Are you sure that the calls on $row after this will not fail hard, if $row is null?

For example at $row->getNumCells().

In essence yes:

RowIteratorInterface::current(): ?Row — the interface contract allows null (vendor/openspout/openspout/src/Reader/RowIteratorInterface.php:15).

CSV RowIterator implementation only returns a Row once valid() is true, and PHP's foreach checks valid() before each current() call — so in practice $row will never be null during normal
iteration (vendor/openspout/openspout/src/Reader/CSV/RowIterator.php:99-102, :73-76).

jeppekroghitk · 2026-06-02T07:49:54Z

+* [PR-308](https://github.com/itk-dev/economics/pull/308)
+  * PHPStan baseline: 530 → 0 in `src/`, bounced to 444 after develop's test suites merged,
+    now 91. Items below are behavior- or schema-affecting and worth calling out.
+  * Migration `Version20260517131038`: 18 columns made nullable on synced entities
+    (`issue`, `project`, `version`, `worklog`) plus 3 DateTime fields, to match property types.
+  * Migration `Version20260517151632`: 11 `NOT NULL` ManyToOne FKs relaxed to `NULL`;
+    application-layer `#[Assert\NotNull]` / form validators still enforce required-ness.
+  * Replaced `?T $prop = null` with non-null defaults on 25 entity properties to satisfy
+    `NOT NULL` columns; `Worker`/`WorkerGroup` `__toString()` switched from `??` to `?:`.
+  * `ForecastReportService` rendered empty epic tags — called non-existent `Epic::getName()`;
+    now uses `Epic::getTitle()`.
+  * `SubscriptionHandlerService::getVersion()` looked up `Version` by non-existent
+    `versionId` field; now uses `find($versionId)`.
+  * `DataProviderService::setTimeSpentSeconds()` silently lost precision passing `float` to an
+    `int` setter; explicit `(int)` cast added.
+  * Report services (`ForecastReportService`, `BillableUnbilledHoursReportService`,
+    `InvoicingRateReportService`) now skip worklogs with null project/issue instead of crashing.
+  * `SubscriptionController::check()` returns HTTP 400 on null `User::getEmail()`;
+    `User::getUserIdentifier()` throws on empty email (Symfony requires `non-empty-string`).
+  * `LeantimeApiService` switched from `json_decode($json, null)` to `json_decode($json, true)`;
+    all `$data->property` accesses converted to array access. Wire shape unchanged.
+  * Removed dead code: `readonly string $id` on 3 report-data models, unreachable `break;` after
+    `throw`, null check on `Row` iterator, unused `$paginator` arg in `SubscriptionRepository`.
+  * Fixed `Epic::getIssues()` PHPDoc typo (`Collection<int, Epic>` → `Collection<int, Issue>`).
+


I get that this is a big PR, but the size of this entry hurts my eyes, along with the overuse of backticks.

tuj added 3 commits May 17, 2026 11:27

7273: Added /** @var Collection<int, X> */ to 20 collection propertie…

a918d19

…s across 9 entity files

Added missing parameter and return types.

a87c372

7273: Added generic-class typehints

46568d3

tuj self-assigned this May 17, 2026

tuj added the enhancement New feature or request label May 17, 2026

tuj added 12 commits May 17, 2026 12:17

7273: Fixed low hanging phpstan issues

ab97436

7273: Fixed trivial misc issues

034d11f

7273: Fixed 16 specific PHPStan-flagged issues, mostly real bugs the …

8745620

…type surface had been hiding

7273: Aligned Doctrine entity property types with database column nul…

07962e6

…lability

7273: Relaxed 11 ManyToOne foreign keys from NOT NULL to NULL to …

46879ef

…match the property types they bind to.

7273: Typed parameters

3257d79

7273: Report services now skip worklogs with no associated project or…

55b61fd

… issue rather than dereferencing nullable getters

7273: Fixed more phpstan raised issues

42bc2bb

7273: Cleared the long tail of PHPStan call-site bugs

d2387ed

7273: Final pass on PHPStan call-site bugs

1a57db2

7273: Changed to array json_decode output

e88fed3

7273: Shortened changelog entries

57b4c62

7273: Changed to asterix

8e7187d

Base automatically changed from feature/psalm-to-phpstan to develop May 20, 2026 08:01

tuj added 8 commits May 20, 2026 10:34

7273: Fixed merge

67acde1

7273: Added MockObject type to mocks

afe5e53

7273: Removed redundant asserts

549158f

7273: Added inline asserts to tests

7310f16

7273: Added asserts

0ce5e1f

7273: Fixed merge

90d9e28

7273: Fixed new baseline issues

8ade5ff

7273: Shortened changelog entry

d19f434

tuj marked this pull request as ready for review June 1, 2026 19:08

tuj requested a review from jeppekroghitk June 2, 2026 05:00

jeppekroghitk approved these changes Jun 2, 2026

View reviewed changes

7273: Cleaned up changelog entry

698f185

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup phpstan baseline issues#308

Cleanup phpstan baseline issues#308
tuj wants to merge 25 commits into
developfrom
feature/phpstan-cleanup-baseline

tuj commented May 17, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented May 17, 2026

Uh oh!

jeppekroghitk left a comment

Uh oh!

jeppekroghitk Jun 2, 2026

Uh oh!

tuj Jun 2, 2026

Uh oh!

jeppekroghitk Jun 2, 2026

Uh oh!

tuj Jun 2, 2026

Uh oh!

jeppekroghitk Jun 2, 2026

Uh oh!

tuj Jun 2, 2026

Uh oh!

jeppekroghitk Jun 2, 2026

Uh oh!

tuj Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tuj commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to ticket

Description

Checklist

Uh oh!

codecov-commenter commented May 17, 2026

Codecov Report

Uh oh!

jeppekroghitk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tuj commented May 17, 2026 •

edited

Loading