Raise legacy PHPStan to level 8 (+ type-bug fixes)#90
Merged
Conversation
`operation:list` rendered the `stop` column by calling `truncateCommand($op->commands['stop'])`. The runtime-operations schema makes `commands.stop` optional (it's `?string` in `Platformsh\Client\Model\Deployment\RuntimeOperation`), and `truncateCommand()` is typed `string $cmd`, so PHP 8 raised a TypeError on any environment that defined an operation without a stop command. Coalesce the value to an empty string before printing or truncating. Integration test added that mocks an operation payload with `"stop": null` and asserts `operation:list` exits cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`AutoscalingSettingsSetCommand::summarizeChangesPerService()` builds the previous-value side of each duration line from `isset(...) ? formatDuration(...) : null`, then passes it to `formatDurationChange()`. The method was typed `int|string` for its first argument, so when an existing autoscaling setting was configured in only one direction (e.g. an `up` trigger but no `down`), `summarizeChangesPerService` raised a TypeError on the missing side. `ResourcesUtil::formatChange()`, which `formatDurationChange()` delegates to, already accepts `int|string|null` and short-circuits on null previous values. Widen the first parameter of `formatDurationChange()` to match. Integration test added: drives `autoscaling:set --metric cpu --duration-down 2m` against a mock whose current `services.app.triggers.cpu` contains only `up`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`resources:set` displays a per-app summary of CPU/memory changes by formatting `$sizeInfo = ResourcesUtil::sizeInfo(...)`. `sizeInfo()` returns null when the deployment's `container_profile` + `profile_size` combination isn't present in the deployment's `container_profiles` map (which can happen when a deprecated profile size remains set on an older deployment). The previous-CPU branch then passed null to `ResourcesUtil::formatCPU(int|float|string)`, raising a TypeError. Guard each `formatCPU` call behind a `$sizeInfo !== null` check, then hand the (possibly null) formatted previous value to `formatChange()`, which already accepts null and prints just the new value when so. Apply the same shape to the memory branch on the symmetric `$newSizeInfo` path. Integration test added that mocks a deployment whose `app` has `profile_size: 0.5` but a `container_profiles` map containing only `0.1`, then asserts `resources:set --size app:0.1 --dry-run` exits cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two paths crashed on activities with a literal null timestamp: - `Model\Activity::getDuration()` called `strtotime($activity->X)` with `$activity->completed_at`, `updated_at` or `created_at` directly, hitting a TypeError in PHP 8 when any of those was null. - `ActivityLoader::load()` constructed `new DateTime($activity->created_at)` to paginate, same TypeError shape. Both happen on `activity:get`, `activity:list`, and any command that streams activities (e.g. backup-await flows) for an activity returned by the API with missing timestamp fields. Gate each call with `!empty(...)`. In `getDuration()` the existing `$end !== false && $start !== false` guard already handles the missing case; in `ActivityLoader::load()` the previous `$startsAt` value is kept when the most recently loaded activity lacks `created_at` (the loop's `$new` check already detects when pagination has stalled). Integration test added that registers a raw API handler returning an activity with `"completed_at": null, "started_at": null, "created_at": null, "updated_at": null` and exercises `activity:list`, `activity:get`, and `activity:get -P duration`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These commands were flagged by raising PHPStan to level 8 (nullable selection / member / environment reaching method calls or DB-typed parameters). On inspection the flagged paths are unreachable at runtime — `loadMemberByEmail()` filters nulls before they reach the caller, the `#domains` env branch only runs when the env is set, and the domain argument is `InputArgument::REQUIRED`. Adding tests that exercise the happy paths documents that and locks them down in case future changes break the upstream guarantees the static type checker can't see. No production code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bumps `legacy/phpstan.neon` from `level: 7` to `level: 8` and regenerates `phpstan-baseline.neon` to suppress the 124 new findings that are not bugs. Background. CLI-145 ran a full pass at level 8 (122 new findings across 57 files at the time) and classified each as bug, annotation, or unclear. Four genuine runtime crashes were fixed in the preceding four commits, with integration coverage. The remaining 124 findings are paths the static type checker can't narrow but that are guarded at runtime — Symfony `Process::getExitCode()` called only after `run()`/`wait()`, `Command::getApplication()` called only inside an `execute()`, `BuildFlavorBase` properties used only after `prepare()`, `preg_replace()` on validated patterns, lazily-init form fields touched only after `configure()`, and so on. Where any of these could plausibly leak in some future refactor the integration tests added in this PR will catch the regression. The Rector visitors (`src/Rector/*`) are also baselined; they are build-time scaffolding and not shipped. A subset of new entries is verified by integration tests added in this PR (domain:update, domain:get, org:user:projects). The rest were reviewed but not driven through end-to-end tests; this baseline gives us a green level-8 gate to ratchet down against. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Raises the legacy PHPStan configuration from level 7 to level 8, addressing newly detected real runtime crashes (null/missing fields from API data) and adding integration coverage for the affected legacy commands. Remaining level-8 static-analysis findings are captured in the regenerated PHPStan baseline.
Changes:
- Harden legacy commands/services against nullable/missing API fields (activities timestamps, runtime operation stop command, missing container profile sizes, autoscaling duration “previous” value).
- Add integration tests that reproduce the previously crashing paths and cover a few “nullable-but-unreachable” findings.
- Bump PHPStan level to 8 and regenerate
phpstan-baseline.neon.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| legacy/src/Service/ActivityLoader.php | Avoid constructing DateTime from empty created_at during activity pagination. |
| legacy/src/Model/Activity.php | Guard strtotime() calls against null timestamps when computing activity duration. |
| legacy/src/Command/RuntimeOperation/ListCommand.php | Tolerate runtime operations without a stop command definition. |
| legacy/src/Command/Resources/ResourcesSetCommand.php | Guard resource change summaries when size info is missing from container profiles. |
| legacy/src/Command/Autoscaling/AutoscalingSettingsSetCommand.php | Allow null “previous duration” when formatting duration changes. |
| legacy/phpstan.neon | Raise PHPStan level from 7 to 8 for legacy analysis. |
| legacy/phpstan-baseline.neon | Update baseline to account for new level-8 findings. |
| integration-tests/runtime_operation_test.go | Add coverage for operation:list and operation:run (including missing stop command). |
| integration-tests/resources_set_test.go | Add coverage for resources:set --dry-run when current profile size is missing from container_profiles. |
| integration-tests/org_user_projects_test.go | Add coverage for organization:user:projects nullable-path findings that are runtime-guarded. |
| integration-tests/domain_update_test.go | Add coverage for project-scoped domain:update / domain:get branches. |
| integration-tests/autoscaling_settings_set_test.go | Add coverage for autoscaling:set when current settings lack a trigger duration. |
| integration-tests/activity_null_timestamp_test.go | Add coverage for null activity timestamps across activity:list and activity:get. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Calling `autoscaling:set --service NAME --duration-up 2m` (or any other scaling option without --metric) reached `validateMetric($metric, ...)` with $metric === null, raising an uncaught TypeError. Trigger path: non-interactive mode, $service set, at least one scaling option set, no --metric. Now the command emits a readable error and exits with code 1, matching the message used when --service is omitted. Verified by integration-tests/autoscaling_validate_metric_test.go, which crashed against the unfixed binary and passes after the fix. Corresponding entry removed from legacy/phpstan-baseline.neon. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ling settings PHPStan level 8 flags two `$current['triggers']` accesses in `summarizeChangesPerService` (AutoscalingSettingsSetCommand.php:599, 601) where $current can be null when $settings[$service] is unset. The trigger path is enabling autoscaling for a service that does not yet appear in the project's autoscaling settings. The CLI does not crash on this path. In PHP 8 the null-offset access emits a warning but yields null, which evaluates as falsy and behaves as expected for a service that does not currently have triggers. The baseline entry for this finding is retained. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AutoscalingSettingsSetCommand assumed $autoscalingSettings['defaults'] was always present and an array. When the autoscaling API returned a settings payload that omitted the "defaults" key (or set it to null), $defaults was null and the next call — getSupportedMetrics($defaults), typed `array $defaults` — raised a TypeError on line 112, crashing the CLI with an unhandled fatal error. Same shape as the null-duration bug fixed in bde012c: a nested API field assumed present at the top of the command, with no guard before downstream code that expects an array. Treat a missing or non-array `defaults` as the API failing to return the information the command needs, print an error, and exit 1. The surrounding code already exits with similar messages when $autoscalingSettings itself is unavailable or the manage-autoscaling link is missing. Integration test added: drives autoscaling:set against a mock that returns an autoscaling-settings payload with no "defaults" key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to 915a95a, which fixed the same null-timestamp crash in `Model\Activity::getDuration()` and `ActivityLoader::load()` but missed `Service\ActivityMonitor`. Two call sites here crashed on an activity whose `created_at` or `started_at` was a literal null from the API: - `getStart()` (line 677) falls back to `strtotime($activity->created_at)` when `started_at` is empty. Reached on every multi-activity wait flow via `waitMultiple()` — `environment:redeploy --wait`, backup-await, snapshot-await, anything streaming activities through the progress bar. - The most-recent-timestamp loop at line 481 calls `strtotime($activity->created_at)` directly. Both now use the same `!empty(...) ? strtotime(...) : false` guard as the earlier fix. Downstream code already handles the `false` return. Integration test added that drives `environment:redeploy --wait` against a mock returning a redeploy activity collection with null timestamps; it reproduced the crash against the unfixed binary and passes after the fix. Surfaced by a skeptical re-review of the level-8 baseline, which the process now requires after a level raise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0f4ad2c fixed the two `strtotime($activity->X)` calls in `Service\ActivityMonitor` but forgot to remove the corresponding baseline entry. PHPStan flags "ignored error pattern not matched" under `lint-phpstan` in CI, which currently fails on PR #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
915a95a guarded the `new DateTime($activity->created_at)` constructor against a null timestamp, but left the pagination cursor unchanged in that case — the next `getActivities()` call would re-request the same window and only stop on the !\$new duplicate-detection check after a redundant round-trip. Break the loop instead: without a valid created_at we can't advance the cursor at all. Per Copilot's review of #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8a15e40 stopped the TypeError when sizeInfo() returned null for the new (requested) size, but did so by silently substituting an empty string, which made the summary render a confusing blank `<info></info>` placeholder. Detect the missing-new-size case explicitly and print a one-line notice that names the size and says CPU/memory details aren't available for it. Per Copilot's review of #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The mock handler ignored the ReadAll() and Unmarshal() errors. If either failed, receivedBody would be unset and downstream assertions could pass or fail for the wrong reason. Per Copilot's review of #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Raises
legacy/phpstan.neonfromlevel: 7tolevel: 8and fixes the four genuine runtime crashes that surfaced along the way. Each fix has an integration test that reproduces the crash on the unfixed binary.The remaining 124 new findings at level 8 are paths the static type checker can't narrow but that are guarded at runtime; they're added to
phpstan-baseline.neon.Commits
fix(legacy): handle runtime operations with no stop command—operation:listcrashed on an operation defined with only astartcommand.fix(legacy): allow null previous value in autoscaling duration change—autoscaling:setcrashed when a metric haduptriggers but nodown(or vice versa).fix(legacy): guard ResourcesSet against missing profile sizes—resources:set --dry-runcrashed when a deployment'sprofile_sizewasn't present in itscontainer_profilesmap (e.g. a deprecated size still set on an older deployment).fix(legacy): tolerate null timestamps on activities—activity:list/activity:getcrashed when the API returned a nullcreated_at,updated_atorcompleted_at.test(integration): cover domain:update, domain:get, org:user:projects— happy-path tests for three commands whose level-8 findings looked nullable but were unreachable.chore(legacy): raise PHPStan to level 8— flips the level and regenerates the baseline.Background
A level-8 analysis produced 122 new findings across 57 files. Each one was inspected by reading the actual call graph rather than trusting the verdict — a skepticism filter that turned out to be important: of eight candidate bugs investigated, only four were real (the four fixed here). The other four had guards the static checker couldn't see (closure invariants, early returns, upstream filtering, Symfony
InputArgument::REQUIRED).🤖 Generated with Claude Code