[#2145] Fixed installer script requiring curl.
#2156
Conversation
WalkthroughIntroduces a dedicated RepositoryDownloader for repository/archive logic, simplifies Downloader to an HTTP file-only downloader, updates InstallCommand to accept both downloaders (removing curl usage), and adjusts tests to the new split. Changes
Sequence Diagram(s)sequenceDiagram
participant IC as InstallCommand
participant RD as RepositoryDownloader
participant FD as File Downloader
participant HTTP as HTTP Client
participant Arch as Archiver
participant FS as Filesystem/Git
IC->>RD: download(repo, ref, dst)
RD->>RD: parseUri(repo)
alt remote repo
RD->>HTTP: request release or archive (discoverLatestReleaseRemote / downloadArchive)
HTTP-->>RD: archive bytes / release data
RD->>FD: download(archiveUrl, tmpFile) %% file downloader used for archive fetch
FD-->>RD: tmpFile path
else local repo
RD->>FS: git archive (archiveFromLocal)
FS-->>RD: tmp archive
end
RD->>Arch: validate & extract(tmpArchive, dst)
Arch-->>FS: extracted files
RD->>RD: validate composer.json in dst
RD-->>IC: resolved version (string)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2156 +/- ##
===========================================
- Coverage 76.35% 75.71% -0.65%
===========================================
Files 109 103 -6
Lines 5676 5526 -150
Branches 44 0 -44
===========================================
- Hits 4334 4184 -150
Misses 1342 1342 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (1)
280-307: Stale test logic: curl is no longer a required tool.This test case still checks for missing
curlcommand (lines 290-292), but the PR removescurlfrom the requirements. The test name "multiple missing tools" is misleading since onlygitwill actually cause a failure now.Consider simplifying this test or removing the curl-related logic:
'Requirements of install command check fails, multiple missing tools' => [ 'command_inputs' => self::tuiOptions([ InstallCommand::OPTION_NO_INTERACTION => TRUE, ]), 'install_executable_finder_find_callback' => function (string $command): ?string { - // Both git and curl commands fails. + // Multiple commands fail (though only git is required). if (str_contains($command, 'git')) { return NULL; } - - if (str_contains($command, 'curl')) { - return NULL; - } - + if (str_contains($command, 'tar')) { + return NULL; + } return '/usr/bin/' . $command; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.vortex/installer/src/Command/InstallCommand.php(7 hunks).vortex/installer/src/Downloader/Downloader.php(1 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(1 hunks).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php(1 hunks).vortex/installer/tests/Functional/Command/InstallCommandTest.php(2 hunks).vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php(2 hunks).vortex/installer/tests/Helpers/TuiOutput.php(0 hunks).vortex/installer/tests/Unit/Downloader/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- .vortex/installer/tests/Helpers/TuiOutput.php
🧰 Additional context used
🧬 Code graph analysis (5)
.vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (3)
.vortex/installer/src/Downloader/Downloader.php (1)
Downloader(14-59).vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-271).vortex/installer/tests/Unit/Downloader/DownloaderTest.php (1)
CoversClass(15-83)
.vortex/installer/tests/Unit/Downloader/DownloaderTest.php (2)
.vortex/installer/src/Downloader/Downloader.php (2)
Downloader(14-59)download(41-57).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
download(31-31)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (2)
.vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-271).vortex/installer/src/Command/InstallCommand.php (1)
setRepositoryDownloader(737-739)
.vortex/installer/src/Downloader/Downloader.php (2)
.vortex/installer/src/Downloader/RepositoryDownloader.php (2)
__construct(36-41)download(43-56).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
download(31-31)
.vortex/installer/src/Command/InstallCommand.php (2)
.vortex/installer/src/Downloader/Downloader.php (2)
Downloader(14-59)download(41-57).vortex/installer/src/Downloader/RepositoryDownloader.php (3)
RepositoryDownloader(17-271)download(43-56)parseUri(58-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
🔇 Additional comments (21)
.vortex/installer/src/Downloader/Downloader.php (1)
14-57: LGTM! Clean separation of concerns for HTTP file downloads.This simplified
Downloaderclass correctly handles basic HTTP file downloads using Guzzle'ssinkoption for streaming. The exception handling properly wraps and preserves the original exception context..vortex/installer/tests/Functional/Command/InstallCommandTest.php (2)
11-11: LGTM! Import updated correctly.The import correctly reflects the new
RepositoryDownloaderclass.
88-92: LGTM! Mock correctly uses new RepositoryDownloader.The test properly mocks
RepositoryDownloaderand usessetRepositoryDownloader()to inject it..vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
10-46: LGTM! Interface appropriately renamed to clarify its purpose.The rename from
DownloaderInterfacetoRepositoryDownloaderInterfacebetter reflects the interface's responsibility for repository-specific downloads..vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (2)
32-32: LGTM! Import updated correctly.
62-62: LGTM! Coverage annotation correctly updated to RepositoryDownloader..vortex/installer/tests/Unit/Downloader/DownloaderTest.php (1)
18-81: LGTM! Comprehensive test coverage for the simplified Downloader.The tests properly cover:
- Successful downloads with mocked client
- Exception handling and error message formatting
- Redirect behavior verification
- Default client instantiation
.vortex/installer/src/Downloader/RepositoryDownloader.php (3)
19-56: LGTM! Clean implementation of repository download logic.The class properly handles both remote and local repositories with appropriate validation for
composer.jsonpresence.
58-93: LGTM! Comprehensive URI parsing with thorough validation.The
parseUrimethod handles multiple URI formats (HTTPS, SSH, file://, local paths) with appropriate reference validation.
216-222: Status code check is unreachable with default Guzzle configuration.Guzzle's default behavior (http_errors = true) throws a RequestException for 4xx/5xx responses regardless of the sink option. The status code check on line 220 will never execute for error responses since an exception will be thrown first.
Either remove the unreachable status code check, or explicitly set
http_errors => falseif you need to handle status codes manually. Addinghttp_errors => true(as suggested in the diff) is redundant since it's already the default.Likely an incorrect or invalid review comment.
.vortex/installer/src/Command/InstallCommand.php (5)
7-8: LGTM! Both downloaders imported for their respective purposes.
Downloaderfor simple HTTP file downloads,RepositoryDownloaderfor repository operations.
79-86: LGTM! Clean separation of downloader responsibilities.The nullable properties with lazy initialization allow for easy dependency injection in tests.
240-252: LGTM!curlcorrectly removed from required commands.This is the key change addressing issue #2145 - the installer no longer requires
curlas an external dependency.
433-434: LGTM! This is the core fix - replacingcurlwith the file downloader.The demo database download now uses the injected
Downloaderinstance instead of shelling out tocurl, making the installer more portable and testable.
727-762: LGTM! Well-structured getter/setter pattern for dependency injection.The lazy initialization with null-coalescing assignment (
??=) is clean and allows for easy test mocking via the public setters..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (6)
23-37: LGTM! Well-structured parameterized test for URI parsing.The test properly handles both success and exception cases using a single test method with conditional expectations.
39-135: LGTM! Comprehensive tests for download operations.Good coverage of success paths, archiver interactions, HTTP errors, and request exceptions.
137-185: LGTM! Thorough testing of release discovery logic.The data provider covers valid releases, drafts, empty responses, and error conditions.
209-231: LGTM! Local download tests with actual Git repository.The test correctly creates a temporary Git repository and handles the special
COMMIT_HASHcase for dynamic commit hash testing.
251-295: LGTM! GitHub token authentication properly tested.Both release discovery and archive download paths verify that the Authorization header is correctly set when
GITHUB_TOKENis present.
297-701: LGTM! Extensive data provider with excellent edge case coverage.The URI parsing data provider comprehensively covers:
- HTTPS, Git SSH, file://, and local path formats
- Valid and invalid reference formats
- Commit hash validation (40-char and 7-char)
- Edge cases like trailing slashes, empty references, and special characters
github-actions
bot
added
the
CONFLICT
AlexSkrypnyk
force-pushed
the
feature/2145-rm-curl-demo
branch
from
fe677f6 to
53ee5d9
Compare
github-actions
bot
removed
the
CONFLICT
AlexSkrypnyk
force-pushed
the
feature/2145-rm-curl-demo
branch
from
53ee5d9 to
67bc8e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.vortex/installer/src/Command/InstallCommand.php(7 hunks).vortex/installer/src/Downloader/Downloader.php(1 hunks).vortex/installer/src/Downloader/RepositoryDownloader.php(1 hunks).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php(1 hunks).vortex/installer/tests/Functional/Command/InstallCommandTest.php(2 hunks).vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php(2 hunks).vortex/installer/tests/Helpers/TuiOutput.php(0 hunks).vortex/installer/tests/Unit/Downloader/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- .vortex/installer/tests/Helpers/TuiOutput.php
🧰 Additional context used
🧬 Code graph analysis (3)
.vortex/installer/tests/Functional/Command/InstallCommandTest.php (3)
.vortex/installer/src/Downloader/Downloader.php (1)
Downloader(14-59).vortex/installer/src/Downloader/RepositoryDownloader.php (1)
RepositoryDownloader(17-273).vortex/installer/src/Command/InstallCommand.php (1)
setRepositoryDownloader(737-739)
.vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (2)
.vortex/installer/src/Downloader/RepositoryDownloader.php (3)
RepositoryDownloader(17-273)parseUri(62-97)download(47-60).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (2)
parseUri(45-45)download(31-31)
.vortex/installer/tests/Unit/Downloader/DownloaderTest.php (3)
.vortex/installer/src/Downloader/Downloader.php (2)
Downloader(14-59)download(41-57).vortex/installer/src/Downloader/RepositoryDownloader.php (1)
download(47-60).vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
download(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
🔇 Additional comments (7)
.vortex/installer/src/Downloader/RepositoryDownloaderInterface.php (1)
7-31: Interface rename and API look consistentThe interface name and method signatures line up cleanly with
RepositoryDownloader, no behavioral or typing concerns here..vortex/installer/tests/Functional/Handlers/BaselineHandlerProcessTest.php (1)
32-63: Functional coverage updated appropriatelyImporting
RepositoryDownloaderand adding it to#[CoversClass]keeps this baseline functional test aligned with the new downloader stack. Looks good..vortex/installer/src/Command/InstallCommand.php (2)
78-87: Downloader dependencies are cleanly injected and test‑friendlySplitting concerns between
RepositoryDownloader(for repo archives) andDownloader(for generic files), with lazy-instantiated getters plussetRepositoryDownloader()/setFileDownloader()for tests, is a solid improvement in structure and testability. No issues here.Also applies to: 718-762
240-252: curl successfully removed from requirements and demo download flow
checkRequirements()now only requiresgit,tar, andcomposer, so the installer no longer hard-fails whencurlis missing.prepareDemo()now downloads the demo database via the injectedDownloader(getFileDownloader()->download($url, $destination)), with the data directory being created beforehand and failures surfaced as exceptions, instead of shelling out tocurl.This matches the PR objective to eliminate direct
curlusage while keeping behavior and failure handling clear..vortex/installer/src/Downloader/RepositoryDownloader.php (1)
39-60: Core repository download flow looks solidRouting between
downloadFromRemote()anddownloadFromLocal()and then asserting the presence ofcomposer.jsonin$dstgives a clear, single responsibility forRepositoryDownloader::download(). With the explicit destination null checks in the branch methods, there are no obvious correctness gaps here..vortex/installer/tests/Unit/Downloader/RepositoryDownloaderTest.php (1)
21-217: RepositoryDownloader is very well covered by unit testsThese tests exercise URI parsing, remote vs local flows, error cases (missing
composer.json, HTTP/archive failures, Git failures), null destinations, and GitHub token propagation in depth. The helpers for creating/removing temp git repos and mocking the collaborators give good confidence in the new downloader.Any minor redundancy (e.g., multiple HEAD remote cases) is acceptable for clarity; no changes needed here.
Also applies to: 237-286, 288-912
.vortex/installer/tests/Unit/Downloader/DownloaderTest.php (1)
18-81: Downloader tests correctly match the new, simplified responsibilityThe tests cover the essential behaviors of the slimmed‑down
Downloader: successful requests with proper options, redirect handling, error translation intoRuntimeException, and instantiation without an injected client. This is aligned with the new implementation and looks good.
| Task::action( | ||
| label: 'Downloading Vortex', | ||
| action: function (): string { | ||
| $version = $this->getDownloader()->download($this->config->get(Config::REPO), $this->config->get(Config::REF), $this->config->get(Config::TMP)); | ||
| $version = $this->getRepositoryDownloader()->download($this->config->get(Config::REPO), $this->config->get(Config::REF), $this->config->get(Config::TMP)); | ||
| $this->config->set(Config::VERSION, $version); | ||
| return $version; | ||
| }, | ||
| hint: fn(): string => sprintf('Downloading from "%s" repository at commit "%s"', ...Downloader::parseUri($this->config->get(Config::REPO))), | ||
| hint: fn(): string => sprintf('Downloading from "%s" repository at commit "%s"', ...RepositoryDownloader::parseUri($this->config->get(Config::REPO))), | ||
| success: fn(string $return): string => sprintf('Vortex downloaded (%s)', $return) | ||
| ); |
There was a problem hiding this comment.
Fix download hint to show the actual configured reference
The download task hint currently derives its repo/ref using:
hint: fn(): string => sprintf(
'Downloading from "%s" repository at commit "%s"',
...RepositoryDownloader::parseUri($this->config->get(Config::REPO)),
),Config::REPO already stores the repo URL/path without the @ref part, and Config::REF stores the actual reference. Re‑parsing just the repo URL makes the hint always show HEAD as the ref, even when the user selected stable or a specific commit. This is misleading but does not affect the actual download, which correctly uses both config values.
You can simplify and make the hint accurate by using the stored config directly:
- hint: fn(): string => sprintf('Downloading from "%s" repository at commit "%s"', ...RepositoryDownloader::parseUri($this->config->get(Config::REPO))),
+ hint: fn(): string => sprintf(
+ 'Downloading from "%s" repository at commit "%s"',
+ $this->config->get(Config::REPO),
+ $this->config->get(Config::REF),
+ ),🤖 Prompt for AI Agents
.vortex/installer/src/Command/InstallCommand.php around lines 153 to 162: the
task hint re-parses Config::REPO which omits the configured ref and thus always
shows HEAD; replace the hint so it uses the stored repo and the actual ref from
Config::REF (i.e., use $this->config->get(Config::REPO) for the repository and
$this->config->get(Config::REF) for the reference) so the message accurately
reflects the selected reference.
| /** | ||
| * Download files from a local or remote Git repository using archive. | ||
| * Download files from URLs using HTTP. | ||
| */ | ||
| class Downloader implements DownloaderInterface { | ||
|
|
||
| const REF_HEAD = 'HEAD'; | ||
|
|
||
| const REF_STABLE = 'stable'; | ||
| class Downloader { | ||
|
|
||
| /** | ||
| * Constructs a new Downloader instance. | ||
| * | ||
| * @param \GuzzleHttp\ClientInterface|null $httpClient | ||
| * Optional HTTP client for testing. If not provided, a default Guzzle | ||
| * client will be created. | ||
| * @param \DrevOps\VortexInstaller\Downloader\ArchiverInterface|null $archiver | ||
| * Optional Archiver instance for testing. If not provided, a default | ||
| * Archiver will be created. | ||
| * @param \DrevOps\VortexInstaller\Utils\Git|null $git | ||
| * Optional Git instance for testing. If not provided, will be created | ||
| * when needed for local repository operations. | ||
| */ | ||
| public function __construct( | ||
| protected ?ClientInterface $httpClient = new Client(['timeout' => 30, 'connect_timeout' => 10]), | ||
| protected ?ArchiverInterface $archiver = new Archiver(), | ||
| protected ?Git $git = NULL, | ||
| ) { | ||
| } |
There was a problem hiding this comment.
Guard against null HTTP client in Downloader
$httpClient is declared as ?ClientInterface but download() unconditionally calls $this->httpClient->request(...). If someone constructs new Downloader(null), this will produce a fatal error.
You can keep the optional-argument ergonomics while ensuring a non-null client internally by normalizing in the constructor:
-class Downloader {
-
- public function __construct(
- protected ?ClientInterface $httpClient = new Client(['timeout' => 30, 'connect_timeout' => 10]),
- ) {
- }
+class Downloader {
+
+ protected ClientInterface $httpClient;
+
+ public function __construct(?ClientInterface $httpClient = NULL) {
+ $this->httpClient = $httpClient ?? new Client(['timeout' => 30, 'connect_timeout' => 10]);
+ }This keeps external behavior the same while removing the null risk.
Also applies to: 41-56
🤖 Prompt for AI Agents
.vortex/installer/src/Downloader/Downloader.php around lines 11 to 26 (and also
ensure same fix for download method at 41-56): the constructor accepts
?ClientInterface but code later calls $this->httpClient->request(...)
unconditionally; normalize the constructor parameter so $this->httpClient is
never null by replacing the nullable assignment with logic that if $httpClient
is null then create and assign a default Guzzle client (with the existing
timeout/connect_timeout settings), ensuring the class property is always a
ClientInterface instance; update any similar initialization paths so download()
can safely call ->request without null checks.
| public static function parseUri(string $src): array { | ||
| if (str_starts_with($src, 'https://')) { | ||
| if (!preg_match('#^(https://[^/]+/[^/]+/[^@]+)(?:@(.+))?$#', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid remote repository format: "%s".', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? static::REF_HEAD; | ||
| } | ||
| elseif (str_starts_with($src, 'git@')) { | ||
| if (!preg_match('#^(git@[^:]+:[^/]+/[^@]+)(?:@(.+))?$#', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid remote repository format: "%s".', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? static::REF_HEAD; | ||
| } | ||
| elseif (str_starts_with($src, 'file://')) { | ||
| if (!preg_match('#^file://(.+?)(?:@(.+))?$#', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid local repository format: "%s".', $src)); | ||
| } | ||
| $repo = $matches[1]; | ||
| $ref = $matches[2] ?? static::REF_HEAD; | ||
| } | ||
| else { | ||
| if (!preg_match('#^(.+?)(?:@(.+))?$#', $src, $matches)) { | ||
| throw new \RuntimeException(sprintf('Invalid local repository format: "%s".', $src)); | ||
| } | ||
| $repo = rtrim($matches[1], '/'); | ||
| $ref = $matches[2] ?? static::REF_HEAD; | ||
| } | ||
|
|
||
| if ($ref != static::REF_STABLE && $ref != static::REF_HEAD && !Validator::gitCommitSha($ref) && !Validator::gitCommitShaShort($ref)) { | ||
| throw new \RuntimeException(sprintf('Invalid reference format: "%s". Supported formats are: %s, %s, %s, %s.', $ref, static::REF_STABLE, static::REF_HEAD, '40-character commit hash', '7-character commit hash')); | ||
| } | ||
|
|
||
| return [$repo, $ref]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
URI parsing is thorough; consider aligning docs with allowed ref formats
parseUri() does a nice job handling HTTPS, SSH, file://, and plain paths, and the validation via Validator::gitCommitSha*() makes the accepted reference formats very explicit (stable, HEAD, or 40/7‑character SHAs).
The help text in InstallCommand::configure() still advertises installing from a “specific release” like @1.2.3, which this parser will now reject as an invalid reference. Either the help text should be updated to reflect the accepted formats, or parseUri() could be extended to support tag-style refs if that use case is still desired.
| protected function discoverLatestReleaseRemote(string $repo_url, ?string $release_prefix = NULL): ?string { | ||
| $path = parse_url($repo_url, PHP_URL_PATH); | ||
| if ($path === FALSE) { | ||
| throw new \RuntimeException(sprintf('Invalid repository URL: "%s".', $repo_url)); | ||
| } | ||
|
|
||
| $path = ltrim((string) $path, '/'); | ||
|
|
||
| $release_url = sprintf('https://api.github.com/repos/%s/releases', $path); | ||
|
|
||
| $headers = ['User-Agent' => 'Vortex-Installer', 'Accept' => 'application/vnd.github.v3+json']; | ||
|
|
||
| $github_token = Env::get('GITHUB_TOKEN'); | ||
| if ($github_token) { | ||
| $headers['Authorization'] = sprintf('Bearer %s', $github_token); | ||
| } | ||
|
|
||
| try { | ||
| $response = $this->httpClient->request('GET', $release_url, ['headers' => $headers]); | ||
| $release_contents = $response->getBody()->getContents(); | ||
| } | ||
| catch (RequestException $e) { | ||
| throw new \RuntimeException(sprintf('Unable to download release information from "%s": %s', $release_url, $e->getMessage()), $e->getCode(), $e); | ||
| } | ||
|
|
||
| if ($release_contents === '' || $release_contents === '0') { | ||
| $message = sprintf('Unable to download release information from "%s"%s.', $release_url, $github_token ? ' (GitHub token was used)' : ''); | ||
| throw new \RuntimeException($message); | ||
| } | ||
|
|
||
| $records = json_decode($release_contents, TRUE); | ||
|
|
||
| foreach ($records as $record) { | ||
| $tag_name = is_scalar($record['tag_name']) ? strval($record['tag_name']) : ''; | ||
| $is_draft = $record['draft'] ?? FALSE; | ||
|
|
||
| if (!$is_draft && (!$release_prefix || str_starts_with($tag_name, $release_prefix))) { | ||
| return $tag_name; | ||
| } | ||
| } | ||
|
|
||
| return NULL; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden JSON handling for release discovery (optional)
discoverLatestReleaseRemote() assumes json_decode() returns an array and iterates $records without checking for decode errors. If GitHub ever returns malformed JSON (or an unexpected structure), this could lead to PHP notices instead of a clean exception.
As a defensive enhancement, you could add:
$records = json_decode($release_contents, TRUE);
if (!is_array($records)) {
throw new \RuntimeException(sprintf('Invalid release data received from "%s".', $release_url));
}before the foreach, to keep error handling consistent with the rest of the class.
🤖 Prompt for AI Agents
.vortex/installer/src/Downloader/RepositoryDownloader.php around lines 153 to
195: the code calls json_decode($release_contents, TRUE) and immediately
iterates $records without validating the result, risking notices or unexpected
behavior on invalid/malformed JSON; update the code to check json_last_error()
and ensure $records is an array (e.g. if (!is_array($records) ||
json_last_error() !== JSON_ERROR_NONE) throw a RuntimeException with a clear
message including the $release_url), so malformed or non-array responses produce
a controlled exception before the foreach.
| use DrevOps\VortexInstaller\Downloader\RepositoryDownloader; | ||
| use DrevOps\VortexInstaller\Runner\ProcessRunner; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Install download failure path correctly migrated to RepositoryDownloader
Using a mocked RepositoryDownloader with setRepositoryDownloader() is the right way to exercise the “Download fails” branch after the refactor; the test wiring remains sound.
Minor nit: in the “multiple missing tools” provider case, the callback and inline comment still talk about both git and curl failing even though checkRequirements() no longer checks for curl. Consider updating that comment (and the str_contains($command, 'curl') branch) to avoid confusion for future readers, since curl is no longer a required tool for the installer.
Also applies to: 88-92, 280-295
🤖 Prompt for AI Agents
.vortex/installer/tests/Functional/Command/InstallCommandTest.php around lines
11-12 (and also adjust occurrences at 88-92 and 280-295): the test provider and
callback still reference curl as a missing tool and contain a
str_contains($command, 'curl') branch, but checkRequirements() no longer
requires curl; update the inline comment and remove or replace the curl-specific
branch so the provider and callback reflect the current behavior (e.g., only
assert/git-related missing-tool behavior), ensuring any mocked command checks
only look for git (or other actual required tools) and the comments describe the
single missing-tool scenario.
Closes #2145
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.