Fixed '--config' flag ignored and multiselect crash in '--no-interaction' mode.#2435
Open
AlexSkrypnyk wants to merge 3 commits intomainfrom
Open
Fixed '--config' flag ignored and multiselect crash in '--no-interaction' mode.#2435AlexSkrypnyk wants to merge 3 commits intomainfrom
AlexSkrypnyk wants to merge 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughAdded a Changes
Sequence DiagramsequenceDiagram
participant User
participant PromptManager as "PromptManager"
participant Handler as "Handler (Abstract/Concrete)"
participant Converter as "Converter"
participant Config as "Config/Env"
User->>PromptManager: call args()
PromptManager->>Config: resolve config/env for handler var
alt env/config null
PromptManager->>Config: try strtoupper(handler::id()) fallback
end
PromptManager->>Handler: supply raw default
PromptManager->>Handler: call normalizeValue(raw_default)
alt string list input
Handler->>Converter: fromList(string_value)
Converter-->>Handler: array
Handler-->>PromptManager: normalized array
else non-string or passthrough
Handler-->>PromptManager: return value unchanged
end
PromptManager->>PromptManager: apply normalized default to prompt args
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/installer/src/Prompts/PromptManager.php:
- Around line 676-679: The current check prevents normalizeValue() from running
for empty-string defaults, so multiselect `''` never becomes `[]`; in
PromptManager.php call `$handler->normalizeValue($default)` before filtering out
`''` (i.e., if `$default` is not null, normalize it first, then only set
`$args['default']` when the normalized value is not an empty string) so
multiselect empty defaults are converted to an empty array and propagated.
In @.vortex/installer/tests/Unit/Handlers/AbstractHandlerNormalizeValueTest.php:
- Around line 25-57: Add a regression test that exercises the full PromptManager
resolution flow — specifically call PromptManager::args(...) to trigger the
fallback-to-short-key logic and ensure PromptManager calls
handler->normalizeValue(...) before assigning prompt defaults (covering the
behavior fixed at the args() fallback + normalization steps). In practice,
create a new test method that constructs Config::fromString('{}') and
PromptManager, invokes $prompt_manager->args(...) with an input that only
matches the handler via its short key (and with values needing normalization,
e.g., comma-separated string for Modules::id() or Services::id()), and assert
the returned args contain the normalized array values (not raw strings) and
correct defaults; reference PromptManager::args, handler->normalizeValue, and a
handler id like Modules::id() or Services::id() to locate the code to exercise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82c9d16e-5875-468f-8659-6cc2c42072d8
📒 Files selected for processing (4)
.vortex/installer/src/Prompts/Handlers/AbstractHandler.php.vortex/installer/src/Prompts/Handlers/HandlerInterface.php.vortex/installer/src/Prompts/PromptManager.php.vortex/installer/tests/Unit/Handlers/AbstractHandlerNormalizeValueTest.php
Comment on lines
+25
to
+57
| #[DataProvider('dataProviderNormalizeValue')] | ||
| public function testNormalizeValue(string $handler_id, mixed $input, mixed $expected): void { | ||
| $config = Config::fromString('{}'); | ||
| $prompt_manager = new PromptManager($config); | ||
| $handlers = $prompt_manager->getHandlers(); | ||
|
|
||
| $this->assertArrayHasKey($handler_id, $handlers); | ||
|
|
||
| $handler = $handlers[$handler_id]; | ||
| $this->assertSame($expected, $handler->normalizeValue($input)); | ||
| } | ||
|
|
||
| public static function dataProviderNormalizeValue(): \Iterator { | ||
| // MultiSelect: comma-separated string converted to array. | ||
| yield 'modules - comma string' => [Modules::id(), 'admin_toolbar,coffee', ['admin_toolbar', 'coffee']]; | ||
| // MultiSelect: single string converted to single-element array. | ||
| yield 'modules - single string' => [Modules::id(), 'admin_toolbar', ['admin_toolbar']]; | ||
| // MultiSelect: array passed through unchanged. | ||
| yield 'modules - array' => [Modules::id(), ['admin_toolbar', 'coffee'], ['admin_toolbar', 'coffee']]; | ||
| // MultiSelect: empty string converted to empty array. | ||
| yield 'modules - empty string' => [Modules::id(), '', []]; | ||
|
|
||
| // Other multiselect handlers. | ||
| yield 'services - single string' => [Services::id(), 'redis', ['redis']]; | ||
| yield 'services - comma string' => [Services::id(), 'redis,solr', ['redis', 'solr']]; | ||
| yield 'deploy_types - single string' => [DeployTypes::id(), 'lagoon', ['lagoon']]; | ||
| yield 'notification_channels - single string' => [NotificationChannels::id(), 'email', ['email']]; | ||
|
|
||
| // Text handler: string passed through unchanged. | ||
| yield 'name - string passthrough' => [Name::id(), 'My Project', 'My Project']; | ||
| // Text handler: array passed through unchanged (not its concern). | ||
| yield 'name - array passthrough' => [Name::id(), ['a'], ['a']]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a PromptManager-level regression test for the fixed path.
This suite validates handler normalization well, but it does not cover the actual resolution flow in PromptManager::args() (Line 651 fallback to short key + Line 677 normalization before prompt default assignment). A focused regression test there would lock in the bugfix behavior end-to-end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/installer/tests/Unit/Handlers/AbstractHandlerNormalizeValueTest.php
around lines 25 - 57, Add a regression test that exercises the full
PromptManager resolution flow — specifically call PromptManager::args(...) to
trigger the fallback-to-short-key logic and ensure PromptManager calls
handler->normalizeValue(...) before assigning prompt defaults (covering the
behavior fixed at the args() fallback + normalization steps). In practice,
create a new test method that constructs Config::fromString('{}') and
PromptManager, invokes $prompt_manager->args(...) with an input that only
matches the handler via its short key (and with values needing normalization,
e.g., comma-separated string for Modules::id() or Services::id()), and assert
the returned args contain the normalized array values (not raw strings) and
correct defaults; reference PromptManager::args, handler->normalizeValue, and a
handler id like Modules::id() or Services::id() to locate the code to exercise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2435 +/- ##
==========================================
- Coverage 79.71% 79.27% -0.44%
==========================================
Files 127 120 -7
Lines 6831 6684 -147
Branches 44 0 -44
==========================================
- Hits 5445 5299 -146
+ Misses 1386 1385 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…iselect handlers.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… to 'Config::fromString()'.
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Code coverage (threshold: 90%) Per-class coverage |
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
1 participant
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
Fixed two bugs that prevented
--configflag values from being applied during--no-interactioninstalls. Config keys stored by handlers use a short ID (e.g.MODULES) butPromptManagerwas only looking them up by the full env var name (e.g.VORTEX_INSTALLER_PROMPT_MODULES), so config values were silently ignored. Additionally, multiselect prompts crashed when receiving a single string value (from an env var or config) because the prompt library expected an array.The fix adds a
normalizeValue()method toHandlerInterfaceandAbstractHandlerthat converts comma-separated strings to arrays forMultiSelecthandlers, and updatesPromptManagerto check both the full env var name and the short handler ID when reading from config.Changes
HandlerInterface.phpAdded
normalizeValue(mixed $value): mixedto the interface. Documents that the method is called on values from config, env vars, and discovery before they are used as prompt defaults.AbstractHandler.phpImplemented
normalizeValue(): when the handler type isMultiSelectand the incoming value is a string, it delegates toConverter::fromList()to split a comma-separated string into an array. All other handlers and non-string values pass through unchanged.PromptManager.phpFixed the config key lookup to fall back to
strtoupper($handler::id())when the full env var name returnsnull. Applied$handler->normalizeValue()to the resolved default before passing it to the prompt arguments.AbstractHandlerNormalizeValueTest.phpNew unit test covering
normalizeValue()acrossMultiSelecthandlers (Modules,Services,DeployTypes,NotificationChannels) and aTexthandler (Name), including comma-separated strings, single strings, arrays, and empty strings.Before / After
Bug 1: Config key mismatch
Bug 2: Multiselect string crash
normalizeValue() dispatch
Summary by CodeRabbit
Release Notes
New Features
Chores