fix(resources:set): stop interactive prompts crashing on numeric defaults#105
Open
pjcdawkins wants to merge 1 commit into
Open
fix(resources:set): stop interactive prompts crashing on numeric defaults#105pjcdawkins wants to merge 1 commit into
pjcdawkins wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a fatal TypeError in interactive flows where Symfony’s QuestionHelper can pass an int default directly into validators under strict_types=1. This aligns the legacy CLI’s interactive prompting behavior with the validators’ intended “cast-to-int then validate” logic.
Changes:
- Widened
validateInstanceCount()/validateDiskSize()parameter types toint|stringinResourcesSetCommand. - Widened
AutoscalingSettingsSetCommand::validateInstanceCount()parameter type toint|stringfor the same interactive-default behavior. - Added an integration test that drives
resources:setinteractively and accepts defaults to prevent regressions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
legacy/src/Command/Resources/ResourcesSetCommand.php |
Accepts int defaults in interactive validators to avoid TypeError under strict types. |
legacy/src/Command/Autoscaling/AutoscalingSettingsSetCommand.php |
Aligns autoscaling instance-count validator with interactive defaults (int) behavior. |
integration-tests/resources_set_interactive_test.go |
Adds regression coverage for accepting interactive defaults without crashing. |
Comments suppressed due to low confidence (1)
legacy/src/Command/Resources/ResourcesSetCommand.php:560
validateDiskSize()documents support for the special values 'default'/'min'/'minimum' (see the--diskoption help), but the current numeric validation runs before those keyword branches. On PHP >= 8 this means non-numeric strings like "default" will fail$size != $valueand throw before reaching the keyword handling.
Consider skipping the numeric validation when the user entered one of the supported keywords (or moving the keyword handling above the numeric validation).
protected function validateDiskSize(int|string $value, string $serviceName, WebApp|Worker|Service $service): int
{
if (!$this->resourcesUtil->supportsDisk($service)) {
throw new InvalidArgumentException(sprintf(
'The %s <error>%s</error> does not support a persistent disk.',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e5f8560 to
7129f7f
Compare
7129f7f to
be42d97
Compare
…ults resources:set and autoscaling:set crashed with a fatal TypeError when the user accepted an interactive default (e.g. "Enter the number of instances", default 1). QuestionHelper::askInput() passed the default - an int - straight to the validator, whose $value is typed string under the command's strict_types. Symfony runs the default through the validator when the user accepts it, so coerce the default to a string inside askInput() when a validator is set. Validators validate typed text, so this matches what typed input provides; it fixes every such prompt at the source and lets validators stay string-typed. PHPStan (level 8) did not catch the original mismatch: the validators are called via a closure with an implicit mixed parameter, which level 8 allows to pass to a string parameter, and the int default only arrives at runtime. Add an interactive integration test covering the resources:set form: accepting the defaults changes nothing, and entering new values submits them in the deployment update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
be42d97 to
bcbea2e
Compare
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.
Problem
upsun resources:set, run interactively, crashes with a fatalTypeErrorthe moment the user presses Enter at the "Enter the number of instances" prompt:The interactive prompts pass the current value as the default straight to the validator when the user accepts it.
instance_countanddiskare ints, but the validators type$valueasstring, and the command file declaresstrict_types=1.autoscaling:sethas the same prompts (instances min/max) and the same crash.Why it got past PHPStan
The validators are invoked through a closure (
fn($v) => $this->validateInstanceCount($v, ...)) whose$vis an implicitmixed. At level 8 PHPStan permits an implicitmixedto flow into astringparameter, and the int default only reaches the validator at runtime via Symfony'sQuestionHelper.Fix
Symfony runs the default through the validator when the user accepts it. Console input is always text, so
QuestionHelper::askInput()now coerces the default to a string when a validator is set. This fixes the whole class of prompt at the source — every validator is guaranteed a string whether the user types a value or accepts the default — and lets the validators stay honestlystring-typed. No call sites change.Test
Adds
integration-tests/resources_set_interactive_test.go, which drivesresources:setinteractively and accepts the defaults. It reproduces theTypeErroragainst the unfixed build and passes with the fix.