LibreSign / LibreSign/libresign

Stop asserting exception messages in PHP tests: define a convention (error codes or typed exceptions)

Open
#8,117 11 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

good first issue php technical debt
Dominant language
PHP
Stars
818
Forks
146
Avg merge
11h 31m
Merged PRs (30d)
326

Description

Follow-up of the discussion in #8092 (https://github.com/LibreSign/libresign/pull/8092#discussion_r3883064993).

Many PHP tests assert exceptions by matching their message. When the message is translatable or just easy to change, editing the text in lib/ silently requires editing the test too, and whoever changes the text (not necessarily a developer) has to know that. The suggestion in #8092 was to assert the error code or the exception class instead of the text.

Current state

Numbers from main today:

  • expectExceptionMessage(): 174 calls in 52 test files (plus 13 expectExceptionMessageMatches()).
  • expectExceptionCode(): 12 calls in 9 files.
  • throw new LibresignException(...): 287 places. 161 of them use a translated message ($l10n->t(...)), 126 use a literal string.
  • Only 51 of the 287 carry a code, and the values mix conventions: 1 (22), 422 (11), 404 (7), 500 (4), 400 (3), Http::STATUS_UNPROCESSABLE_ENTITY (3) and Http::STATUS_FORBIDDEN (1). LibresignException itself defines no code constants, and the code is exposed to clients through ErrorPayloadBuilder (code) and SignFileService (error_code), so any convention also touches the API contract.
  • 8 test files assert a translated message while building the real IL10N from the factory, so they depend on the test locale.
  • There are already typed subclasses in lib/Exception/ (InvalidPasswordException, InvalidSignatureException, EmptyCertificateException, ...), but most throw sites still use the base LibresignException.

What should be decided first

Two directions came up in #8092. They are not exclusive, but a convention is needed before touching many files:

  1. Error codes: define constants for the codes (a dedicated class, like JSActions, following the "HTTP-like plus one digit" idea to avoid confusion with HTTP status codes), use them at the throw sites, and assert them with expectExceptionCode().
  2. Typed exceptions: extend the existing lib/Exception/ subclasses so tests assert the class with expectException().

Please use this issue to settle the convention (and decide what to do with the existing 1/4xx codes), then the work can be split into small PRs.

How to contribute

[!IMPORTANT]
Do not solve this issue in one PR.

Like #8053, each PR should cover one or two related source/test pairs, reference this issue without Fixes/Closes, and keep the diff small. Please comment here with the files you want to work on before starting.

Suggested order:

  1. Tests that assert translated messages ($l10n->t(...) in lib/, real IL10N in the test), because they break on any wording change.
  2. Tests that assert literal messages in business rules (signing, certificates, policies, permissions, validation, identity flows).
  3. The remaining message assertions.

Do not change production behavior (messages shown to users, API payloads) only to make a test easier; when a code or a class is added, keep the message as is.

Effort

The complete issue is medium-to-high effort because it covers many files, but each PR should be low effort.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the discussion in #8092, then inspect the exception subclasses in lib/Exception/, message assertions across the PHP test files, and the code handling in ErrorPayloadBuilder and SignFileService. First document a convention for error codes or typed exceptions, including existing codes; done means the decision supports small source/test pair PRs without changing user-facing messages or API behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend-api-design, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.