Skip to content

code-quality: Plugin identifies checksumMissing by string-equality of exception messages; catch branches are duplicated #184

Description

@s2x

Description

After #48 / PR #181, the Composer Plugin distinguishes a checksum refusal from other DownloadExceptions by comparing exception messages as strings:

if ($e->getMessage() === DownloadException::checksumMissing($binaryName)->getMessage())

This is correct today (checksumMissing() is a pure sprintf factory and the same $binaryName local is used in both the throw and catch sites, so the messages are byte-identical), but it is fragile under future refactoring: if the message format ever changes (e.g. adding the version, URL, or a timestamp), the comparison silently breaks and the exception falls through to the generic "download failed" branch — a UX regression where users no longer see the actionable "add extra.scanmephp.checksums" hint. This is not a security issue (the exception is caught either way; fail-closed holds), only a maintainability/UX concern.

Additionally, the catch (DownloadException $e) else-branch and the catch (\Exception $e) branch in both install methods produce byte-identical output ("⚠️ … download failed: …" + the fallback line). The duplication is intentional but the two copies diverge silently if one is changed.

Where

  • src/Composer/Plugin.php:176 (extension) and Plugin.php:234 (FFI) — message-equality check
  • src/Composer/Plugin.php:182-184 vs 186-189 (extension), 240-241 vs 243-245 (FFI) — duplicated catch-branch output
  • src/Exception/DownloadException.php:22-28 — the checksumMissing() factory whose format the comparison depends on

Suggested fix

Decouple detection from the message format. Any of:

  • an exception code constant: public const CODE_CHECKSUM_MISSING = 1; set in checksumMissing(), checked with \->getCode() === DownloadException::CODE_CHECKSUM_MISSING
  • a dedicated subclass ChecksumMissingException extends DownloadException caught by type
  • an instance method public function isChecksumMissing(): bool

Then unify the non-checksum DownloadException and generic \Exception handling so the "download failed" text exists in one place per install method.

Verification

Verified against current main (merge 4864eac) during the #48 work cycle (review-critical round 2, finding A + B). Not tracked by any existing issue.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    code-qualityJakość kodu, analiza statyczna, refaktoryseverity:lowDrobny problem, higiena kodu

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions