diff --git a/psalm-baseline.xml b/psalm-baseline.xml index ca11921..a5f7c4f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + @@ -13,30 +13,9 @@ - - - - - $toDownload[$software] - ?? DownloadConfig::fromSoftwareId((string) $software), - $input->getArgument(self::ARG_SOFTWARE), - )]]> - - - - getArgument(self::ARG_SOFTWARE)]]> - - - - - - - ]]> - @@ -142,14 +121,6 @@ - - - - - - - - class()]]> @@ -240,15 +211,6 @@ - - - repositories as $repository) { - yield from $repository->getReleases(); - } - }]]> - - - - static::create($items())]]> - - - - - @@ -312,17 +267,6 @@ - - - - - - - - @@ -358,26 +302,6 @@ - - - ]]> - - - - - assets as $assetDTO) { - yield GitHubAsset::fromDTO($api, $result, $assetDTO); - } - }]]> - - - assets === null]]> - - - assets === null]]> - - valid() ? $loader->current() : []]]> @@ -400,35 +324,13 @@ - - - - - - - - - - - - - - - - - - - - - - diff --git a/src/Command/Base.php b/src/Command/Base.php index 86411b4..3641574 100644 --- a/src/Command/Base.php +++ b/src/Command/Base.php @@ -46,11 +46,6 @@ abstract class Base extends Command public static function getCommandName(): ?string { - if (!\class_exists(AsCommand::class)) { - // Fall back on lower Symfony versions - return self::getDefaultName(); - } - if ($attributes = (new \ReflectionClass(static::class))->getAttributes(AsCommand::class)) { /** @var AsCommand $attribute */ $attribute = $attributes[0]->newInstance(); diff --git a/src/Command/Get.php b/src/Command/Get.php index 7653508..6af5e93 100644 --- a/src/Command/Get.php +++ b/src/Command/Get.php @@ -170,14 +170,22 @@ private static function getDownloadActions(InputInterface $input, Actions $actio $toDownload[$action->software] = $action; } - $destinationPath = $input->getOption('path'); + /** @var mixed $path */ + $path = $input->getOption('path'); + $destinationPath = \is_string($path) && $path !== '' ? $path : null; + + /** @var list $names */ + $names = \array_values(\array_filter( + (array) $input->getArgument(self::ARG_SOFTWARE), + static fn(mixed $name): bool => \is_string($name) && $name !== '', + )); return \array_map( static fn(string $software): DownloadConfig => $toDownload[$software] ?? self::parseSoftware( $software, $destinationPath, ), - (array) $input->getArgument(self::ARG_SOFTWARE), + $names, ); } diff --git a/src/Module/Archive/Internal/GzArchive.php b/src/Module/Archive/Internal/GzArchive.php index acac3ad..b5c6e1a 100644 --- a/src/Module/Archive/Internal/GzArchive.php +++ b/src/Module/Archive/Internal/GzArchive.php @@ -16,6 +16,9 @@ */ final class GzArchive extends Archive { + /** + * @return \Generator + */ public function extract(): \Generator { $sourcePath = $this->asset->getRealPath() ?: $this->asset->getPathname(); @@ -27,7 +30,8 @@ public function extract(): \Generator try { // Derive output filename by stripping .gz extension - $outputName = \preg_replace('/\.gz$/i', '', $this->asset->getFilename()); + $fileName = $this->asset->getFilename(); + $outputName = \preg_replace('/\.gz$/i', '', $fileName) ?? $fileName; $tempPath = \sys_get_temp_dir() . \DIRECTORY_SEPARATOR . $outputName; $out = \fopen($tempPath, 'wb'); @@ -50,7 +54,7 @@ public function extract(): \Generator $fileInfo = new \SplFileInfo($tempPath); /** @var \SplFileInfo|null $fileTo */ - $fileTo = yield $fileInfo->getPathname() => $fileInfo; + $fileTo = yield $tempPath => $fileInfo; if ($fileTo instanceof \SplFileInfo) { \copy($tempPath, $fileTo->getRealPath() ?: $fileTo->getPathname()); diff --git a/src/Module/HttpClient/StreamReader.php b/src/Module/HttpClient/StreamReader.php new file mode 100644 index 0000000..98978c1 --- /dev/null +++ b/src/Module/HttpClient/StreamReader.php @@ -0,0 +1,47 @@ + + */ + public static function chunks( + StreamInterface $stream, + ?\Closure $progress = null, + int $chunkSize = 8192, + ): \Generator { + $size = $stream->getSize(); + $loaded = 0; + + while (!$stream->eof()) { + $chunk = $stream->read($chunkSize); + if ($chunk === '') { + break; + } + + $loaded += \strlen($chunk); + $progress === null or $progress($loaded, $size, []); + + yield $chunk; + } + } +} diff --git a/src/Module/Repository/Internal/Collection.php b/src/Module/Repository/Internal/Collection.php index 8afb1b3..b8033b0 100644 --- a/src/Module/Repository/Internal/Collection.php +++ b/src/Module/Repository/Internal/Collection.php @@ -60,7 +60,9 @@ final public function __construct( * * @template TNew * - * @param iterable $items Source of items + * @param iterable|(\Closure(): iterable)|mixed $items Source of items. A closure is + * called once and its result converted, which lets a caller defer building the items. + * Anything else throws, so the parameter stays `mixed` for the checks below. * @return static New collection instance * @throws \InvalidArgumentException If the input cannot be converted to a collection */ diff --git a/src/Module/Repository/Internal/GitHub/GitHubAsset.php b/src/Module/Repository/Internal/GitHub/GitHubAsset.php index f90b109..020916c 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubAsset.php +++ b/src/Module/Repository/Internal/GitHub/GitHubAsset.php @@ -8,6 +8,7 @@ use Internal\DLoad\Module\Common\Architecture; use Internal\DLoad\Module\Common\OperatingSystem; use Internal\DLoad\Module\HttpClient\Method; +use Internal\DLoad\Module\HttpClient\StreamReader; use Internal\DLoad\Module\Repository\Internal\Asset; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\Response\AssetInfo; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\RepositoryApi; @@ -54,23 +55,14 @@ public static function fromDTO( * it MUST be called on DNS resolution, on arrival of headers and on completion; * it SHOULD be called on upload/download of data and at least 1/s * - * @return \Generator + * @return \Generator * @throws RepositoryException */ public function download(?\Closure $progress = null): \Generator { $response = $this->api->request(Method::Get, $this->getUri()); - $body = $response->getBody(); - $size = $body->getSize(); - $loaded = 0; - - while (!$body->eof()) { - $chunk = $body->read(8192); - $loaded += \strlen($chunk); - $progress === null or $progress($loaded, $size, []); - yield $chunk; - } + yield from StreamReader::chunks($response->getBody(), $progress); } public function destroy(): void diff --git a/src/Module/Repository/Internal/GitHub/GitHubRelease.php b/src/Module/Repository/Internal/GitHub/GitHubRelease.php index 5951ce9..8f0aef9 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRelease.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRelease.php @@ -47,9 +47,15 @@ public static function fromDTO( return $result; } + /** + * `Destroyable` requires this to be idempotent, and the `unset()` below leaves `$assets` + * uninitialized — a state Psalm does not model for a typed property, hence the suppression. + * + * @psalm-suppress RedundantPropertyInitializationCheck + */ public function destroy(): void { - $this->assets === null or $this->assets->map( + isset($this->assets) and $this->assets->map( static fn(object $asset) => $asset instanceof Destroyable and $asset->destroy(), ); diff --git a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php index fab47a3..6af6c71 100644 --- a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php +++ b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php @@ -33,6 +33,9 @@ final class RepositoryApi */ public readonly string $repositoryPath; + /** + * @param non-empty-string $projectPath + */ public function __construct( private readonly Client $client, private readonly HttpFactory $httpFactory, @@ -71,11 +74,11 @@ public function getRepository(): RepositoryInfo $response = $this->request(Method::Get, \sprintf(self::URL_REPOSITORY, \urlencode($this->repositoryPath))); /** @var array{ - * name: string, - * name_with_namespace: string, + * name: non-empty-string, + * name_with_namespace: non-empty-string, * description: string|null, - * web_url: string, - * visibility: bool, + * web_url: non-empty-string, + * visibility: string, * created_at: string, * updated_at: string * } $data */ diff --git a/src/Module/Repository/Internal/GitLab/Api/Response/AssetInfo.php b/src/Module/Repository/Internal/GitLab/Api/Response/AssetInfo.php index 65544eb..46512ea 100644 --- a/src/Module/Repository/Internal/GitLab/Api/Response/AssetInfo.php +++ b/src/Module/Repository/Internal/GitLab/Api/Response/AssetInfo.php @@ -35,7 +35,7 @@ public static function fromApiResponse(array $data): self { return new self( name: $data['name'], - downloadUrl: !empty($data['direct_asset_url']) ? $data['direct_asset_url'] : $data['url'], + downloadUrl: $data['direct_asset_url'] ?? $data['url'], linkType: $data['link_type'] ?? null, ); } diff --git a/src/Module/Repository/Internal/GitLab/Api/Response/RepositoryInfo.php b/src/Module/Repository/Internal/GitLab/Api/Response/RepositoryInfo.php index a0b2f32..1cdd5ae 100644 --- a/src/Module/Repository/Internal/GitLab/Api/Response/RepositoryInfo.php +++ b/src/Module/Repository/Internal/GitLab/Api/Response/RepositoryInfo.php @@ -29,11 +29,11 @@ public function __construct( /** * @param array{ - * name: string, - * name_with_namespace: string, + * name: non-empty-string, + * name_with_namespace: non-empty-string, * description: string|null, - * web_url: string, - * visibility: bool, + * web_url: non-empty-string, + * visibility: string, * created_at: string, * updated_at: string * } $data diff --git a/src/Module/Repository/Internal/GitLab/Factory.php b/src/Module/Repository/Internal/GitLab/Factory.php index e34ffe8..68d601d 100644 --- a/src/Module/Repository/Internal/GitLab/Factory.php +++ b/src/Module/Repository/Internal/GitLab/Factory.php @@ -45,7 +45,8 @@ public function supports(RepositoryConfig $config): bool public function create(RepositoryConfig $config): GitLabRepository { - $uri = \parse_url($config->uri, PHP_URL_PATH) ?? $config->uri; + $path = \parse_url($config->uri, PHP_URL_PATH); + $uri = \is_string($path) && $path !== '' ? $path : $config->uri; $api = $this->createRepositoryApi($uri); return new GitLabRepository($api, $uri, $this->logger); diff --git a/src/Module/Repository/Internal/GitLab/GitLabAsset.php b/src/Module/Repository/Internal/GitLab/GitLabAsset.php index 9c6a97a..5b0f7a7 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabAsset.php +++ b/src/Module/Repository/Internal/GitLab/GitLabAsset.php @@ -7,6 +7,7 @@ use Internal\Destroy\Destroyable; use Internal\DLoad\Module\Common\Architecture; use Internal\DLoad\Module\Common\OperatingSystem; +use Internal\DLoad\Module\HttpClient\StreamReader; use Internal\DLoad\Module\Repository\Internal\Asset; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\Response\AssetInfo; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; @@ -53,23 +54,14 @@ public static function fromDTO( * it MUST be called on DNS resolution, on arrival of headers and on completion; * it SHOULD be called on upload/download of data and at least 1/s * - * @return \Generator + * @return \Generator * @throws RepositoryException */ public function download(?\Closure $progress = null): \Generator { $response = $this->api->downloadArtifact($this->release->getRepository()->getName(), $this->release->getName(), $this->getName()); - $body = $response->getBody(); - $size = $body->getSize(); - $loaded = 0; - - while (!$body->eof()) { - $chunk = $body->read(8192); - $loaded += \strlen($chunk); - $progress === null or $progress($loaded, $size, []); - yield $chunk; - } + yield from StreamReader::chunks($response->getBody(), $progress); } public function destroy(): void diff --git a/src/Module/Repository/Internal/GitLab/GitLabRelease.php b/src/Module/Repository/Internal/GitLab/GitLabRelease.php index d3f655b..f4bc854 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRelease.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRelease.php @@ -47,9 +47,15 @@ public static function fromDTO( return $result; } + /** + * `Destroyable` requires this to be idempotent, and the `unset()` below leaves `$assets` + * uninitialized — a state Psalm does not model for a typed property, hence the suppression. + * + * @psalm-suppress RedundantPropertyInitializationCheck + */ public function destroy(): void { - $this->assets === null or $this->assets->map( + isset($this->assets) and $this->assets->map( static fn(object $asset) => $asset instanceof Destroyable and $asset->destroy(), ); diff --git a/src/Module/Velox/Internal/Config/Pipeline/TomlData.php b/src/Module/Velox/Internal/Config/Pipeline/TomlData.php index 8c85cea..1b9b41d 100644 --- a/src/Module/Velox/Internal/Config/Pipeline/TomlData.php +++ b/src/Module/Velox/Internal/Config/Pipeline/TomlData.php @@ -21,7 +21,7 @@ final class TomlData /** * Creates a new immutable TOML data container. * - * @param array $data The configuration data array + * @param array $data The configuration data array. TOML keys are always strings. */ public function __construct( private readonly array $data = [], @@ -38,7 +38,12 @@ public function __construct( */ public static function fromString(string $toml): self { - return new self(Toml::parseToArray($toml)); + # `Toml::parseToArray()` is declared as a bare `array`; a TOML document is a table, so its + # top-level keys are strings by construction. + /** @var array $data */ + $data = Toml::parseToArray($toml); + + return new self($data); } /** diff --git a/tests/Unit/Module/Archive/Internal/GzArchiveTest.php b/tests/Unit/Module/Archive/Internal/GzArchiveTest.php new file mode 100644 index 0000000..0d23718 --- /dev/null +++ b/tests/Unit/Module/Archive/Internal/GzArchiveTest.php @@ -0,0 +1,119 @@ + Paths to remove once the test is done. */ + private array $garbage = []; + + public static function provideArchiveNames(): \Generator + { + yield 'lowercase extension' => ['payload.txt.gz', 'payload.txt']; + yield 'uppercase extension' => ['payload.txt.GZ', 'payload.txt']; + yield 'no extension to strip' => ['payload', 'payload']; + yield 'gz inside the name' => ['payload.gz.bin.gz', 'payload.gz.bin']; + } + + #[Test] + public function extractYieldsTheDecompressedFile(): void + { + $archive = $this->gzArchive('payload.txt.gz', 'compressed content'); + + $generator = $archive->extract(); + $extracted = $generator->current(); + + Assert::same(\file_get_contents($extracted->getPathname()), 'compressed content'); + } + + #[Test] + public function extractKeysTheFileByItsOwnPath(): void + { + $archive = $this->gzArchive('payload.txt.gz', 'compressed content'); + + $generator = $archive->extract(); + + Assert::same($generator->key(), $generator->current()->getPathname()); + } + + #[DataProvider('provideArchiveNames')] + #[Test] + public function extractStripsTheGzExtensionFromTheOutputName(string $archiveName, string $expectedName): void + { + $archive = $this->gzArchive($archiveName, 'compressed content'); + + $generator = $archive->extract(); + + Assert::same($generator->current()->getFilename(), $expectedName); + } + + #[Test] + public function extractCopiesTheFileToTheDestinationSentBack(): void + { + $archive = $this->gzArchive('payload.txt.gz', 'compressed content'); + $destination = $this->workDir . \DIRECTORY_SEPARATOR . 'unpacked.txt'; + + $generator = $archive->extract(); + $generator->current(); + $generator->send(new \SplFileInfo($destination)); + + Assert::true(\is_file($destination)); + Assert::same(\file_get_contents($destination), 'compressed content'); + } + + #[BeforeTest] + protected function prepare(): void + { + $this->workDir = \sys_get_temp_dir() . \DIRECTORY_SEPARATOR . 'dload-gz-' . \uniqid(); + \mkdir($this->workDir, 0777, true); + $this->garbage[] = $this->workDir; + } + + #[AfterTest] + protected function cleanup(): void + { + foreach ($this->garbage as $path) { + \is_dir($path) ? self::removeDirectory($path) : (\is_file($path) and \unlink($path)); + } + + $this->garbage = []; + } + + private static function removeDirectory(string $directory): void + { + foreach (\array_diff((array) \scandir($directory), ['.', '..']) as $entry) { + $path = $directory . \DIRECTORY_SEPARATOR . $entry; + \is_dir($path) ? self::removeDirectory($path) : \unlink($path); + } + + \rmdir($directory); + } + + /** + * Writes a real gzip archive and wraps it, registering the file the extraction will leave in + * the system temp directory for cleanup. + */ + private function gzArchive(string $archiveName, string $content): GzArchive + { + $archivePath = $this->workDir . \DIRECTORY_SEPARATOR . $archiveName; + \file_put_contents($archivePath, (string) \gzencode($content)); + + $outputName = \preg_replace('/\.gz$/i', '', $archiveName); + $this->garbage[] = \sys_get_temp_dir() . \DIRECTORY_SEPARATOR . $outputName; + + return new GzArchive(new \SplFileInfo($archivePath)); + } +} diff --git a/tests/Unit/Module/HttpClient/StreamReaderTest.php b/tests/Unit/Module/HttpClient/StreamReaderTest.php new file mode 100644 index 0000000..6771918 --- /dev/null +++ b/tests/Unit/Module/HttpClient/StreamReaderTest.php @@ -0,0 +1,95 @@ + 2) { + break; + } + } + + Assert::same($chunks, ['only']); + } + + #[Test] + public function chunkSizeIsPassedToTheStream(): void + { + $stream = new StreamStub(['payload']); + + \iterator_to_array(StreamReader::chunks($stream, chunkSize: 16)); + + Assert::same($stream->readLengths, [16]); + } +} diff --git a/tests/Unit/Module/HttpClient/Stub/StreamStub.php b/tests/Unit/Module/HttpClient/Stub/StreamStub.php new file mode 100644 index 0000000..cfe6435 --- /dev/null +++ b/tests/Unit/Module/HttpClient/Stub/StreamStub.php @@ -0,0 +1,107 @@ + Lengths every `read()` was called with, in order. */ + public array $readLengths = []; + + /** @var list */ + private array $chunks; + + /** + * @param list $chunks Chunks to hand out, in order. + * @param bool $eofWhenDrained Whether `eof()` flips to true once the script is exhausted. + * `false` reproduces a stream that never admits to being finished. + * @param int|null $size Value reported by `getSize()`. + */ + public function __construct( + array $chunks = [], + private readonly bool $eofWhenDrained = true, + private readonly ?int $size = null, + ) { + $this->chunks = $chunks; + } + + public function read(int $length): string + { + $this->readLengths[] = $length; + + return \array_shift($this->chunks) ?? ''; + } + + public function eof(): bool + { + return $this->eofWhenDrained && $this->chunks === []; + } + + public function getSize(): ?int + { + return $this->size; + } + + public function getContents(): string + { + $contents = $this->__toString(); + $this->chunks = []; + + return $contents; + } + + public function close(): void {} + + public function detach() + { + return null; + } + + public function tell(): int + { + return 0; + } + + public function isSeekable(): bool + { + return false; + } + + public function seek(int $offset, int $whence = SEEK_SET): void {} + + public function rewind(): void {} + + public function isWritable(): bool + { + return false; + } + + public function write(string $string): int + { + return 0; + } + + public function isReadable(): bool + { + return true; + } + + public function getMetadata(?string $key = null) + { + return null; + } + + public function __toString(): string + { + return \implode('', $this->chunks); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php new file mode 100644 index 0000000..96b9527 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php @@ -0,0 +1,74 @@ + ['gitlab', true]; + yield 'mixed case' => ['GitLab', true]; + yield 'uppercase' => ['GITLAB', true]; + yield 'github' => ['github', false]; + yield 'unknown' => ['custom', false]; + } + + public static function provideRepositoryUris(): \Generator + { + yield 'bare project path' => ['group/project', 'group/project']; + yield 'full url' => ['https://gitlab.com/group/project', '/group/project']; + yield 'nested group' => ['https://gitlab.com/group/sub/project', '/group/sub/project']; + + # `parse_url()` returns null for a URL without a path and false for one it cannot parse at + # all; both have to fall back to the configured value rather than be passed on. + yield 'url without a path' => ['https://gitlab.com', 'https://gitlab.com']; + yield 'unparsable url' => ['http://:80', 'http://:80']; + yield 'scheme only' => ['https://', 'https://']; + } + + #[DataProvider('provideSupportedTypes')] + #[Test] + public function supportsOnlyGitlabRegardlessOfCase(string $type, bool $expected): void + { + $config = new RepositoryConfig(); + $config->type = $type; + $config->uri = 'group/project'; + + Assert::same($this->factory->supports($config), $expected); + } + + #[DataProvider('provideRepositoryUris')] + #[Test] + public function createDerivesTheProjectPathFromTheUri(string $uri, string $expectedPath): void + { + $config = new RepositoryConfig(); + $config->type = 'gitlab'; + $config->uri = $uri; + + $repository = $this->factory->create($config); + + Assert::same($repository->getName(), $expectedPath); + } + + #[BeforeTest] + protected function prepare(): void + { + $this->factory = new Factory(new HttpFactoryStub(), new GitLabConfig(), new Logger()); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitLab/Stub/HttpFactoryStub.php b/tests/Unit/Module/Repository/Internal/GitLab/Stub/HttpFactoryStub.php new file mode 100644 index 0000000..b71c719 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitLab/Stub/HttpFactoryStub.php @@ -0,0 +1,38 @@ +shouldIgnoreMissing(); + } + + public function request( + string|Method $method, + string|UriInterface $uri, + array $headers = [], + ): RequestInterface { + return \Mockery::mock(RequestInterface::class)->shouldIgnoreMissing(); + } + + public function client(): ClientInterface + { + return \Mockery::mock(ClientInterface::class)->shouldIgnoreMissing(); + } +}