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

Follow-up of the discussion in #8092 (#8092 (comment)).

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:

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().
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:

Tests that assert translated messages ($l10n->t(...) in lib/, real IL10N in the test), because they break on any wording change.
Tests that assert literal messages in business rules (signing, certificates, policies, permissions, validation, identity flows).
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.

Hi, I'd like to work on this. Per your guidance I'll keep it small starting with one or two source/test pairs from the "tests asserting translated messages" category first, converting them to typed exceptions or error-code assertions rather than message-string matching.
I'll open a separate PR per pair and reference this issue (not Fixes/Closes) as you asked.
If there's a specific file or exception class you'd like tackled first, let me know, otherwise I'll pick one from that first category and go from there.

@ajayraho

@ajayraho, since the suggestion is to avoid making a very large PR, I suggest you identify the first set of behaviors you'll be working on an share here before start the work. By this way, will be possible to more people to work on this issue.

Hi, per your ask, here's the specific pair I'd like to start with, so it's visible for others to pick different ones:
lib/Service/File/Pdf/PdfParser.php (line 43) and lib/Service/File/Pdf/PdfValidator.php (line 48), both throw LibresignException($this->l10n->t(...)) with translated, filename-interpolated messages, and their tests (PdfParserTest.php, PdfValidatorTest.php) each assert a filename substring via expectExceptionMessage().

The plan: to introduce typed exceptions following the existing convention in lib/Exception/ (e.g. InvalidPasswordException), something like an UnprocessablePdfException for the "unable to process as PDF" case and update the two tests to assert via expectException(...) on the type instead of matching message text.
Happy to use error codes instead if you'd prefer that convention here; typed exceptions just seemed to match what's already in the codebase.

Will open a PR scoped to just these two once you're good with the approach.

I prefer typed exceptions over error codes whenever it's possible to use an existing native PHP exception, an OCP exception, or an existing LibreSign exception, rather than creating many custom exceptions and increasing the size of the codebase. Does you have other architecture suggestion or other point of view?

About the PR size, don't worry about add multiple changes into a PR only if don't make the PR very big and complex to review. I think that a good approach is:

Up to 200 changed lines: ideal or healthy size.
200–400 lines: caution range; consider splitting.
More than 400 lines: considered large; split whenever possible.
More than 600–1,000 lines: generally treated as an exception.

Thanks for the guidance, makes sense on avoiding exception sprawl.

For this specific pair, I actually don't think either native/OCP or an existing typed LibreSign exception fits cleanly:
LibresignException is relied on directly by InjectionMiddleware and several controllers (instanceof LibresignException, jsonSerialize()) for API error formatting, so switching to a native exception here would break that path, and none of the existing typed subclasses (PageException, InvalidSignatureException, etc.) are a semantic match.

What I did find: InjectionMiddleware::getStatusCodeFromException() already maps $exception->getCode() straight to the HTTP response status (defaulting to 422 when unset). So error codes aren't just a testing convention here, they're already load-bearing in production, just unused by these two throw sites.
My proposal: keep LibresignException as-is (no new class), pass an explicit code at both throw sites (e.g. Http::STATUS_UNPROCESSABLE_ENTITY), and update the two tests to expectExceptionCode(...) instead of matching message text. Zero new files, ~10-15 line diff.
Let me know if that lines up with what you had in mind, or if you'd rather define named constants for these specific error cases instead of reusing the HTTP status directly.

Go ahead and use your feeling. Isn't a real block about the PR size, is only a good practice.

refactor(exception): add error codes to the invalid token and gateway…

After #8103, I think we should clarify one important point in this issue before more PRs follow the same pattern.

We now have two different concepts that are currently sharing Exception::getCode():

a semantic error code, used to identify a specific LibreSign error;
an HTTP status code, used to build the HTTP response.

Because of this, I don't think new tests should use values such as 422 as the identity of an error. Many unrelated errors can return the same HTTP status.

I think the convention should be:

prefer an existing typed exception when it already represents the error well;
otherwise use a semantic error code;
when possible, keep the semantic error constant close to the class that produces the error, instead of adding many unrelated constants to LibresignException;
avoid creating a new exception class only to make a test identify one specific error;
keep the HTTP status and the semantic error identity as separate concepts.

#8103 introduced the semantic error code idea, but I think we should also review where those constants should live before this pattern is used in more places.

Agreed on keeping each code next to the class that raises the error. The two constants from #8103 were the only ones on LibresignException, so I opened #8196 moving them to AbstractIdentifyMethod::CODE_INVALID_TOKEN and TwofactorGatewayService::CODE_NOT_ENABLED (values unchanged). With that, LibresignException has no error-code constants left and the layout matches #8172.

refactor(exception): keep the semantic error codes next to the classe…

I'd like to take this pair next — they are the files I've just been through in #8274/#8275/#8276, so the behaviors are fresh:

lib/Handler/SignEngine/Pkcs12Handler.php and its parent lib/Handler/SignEngine/SignEngineHandler.php
tests/php/Unit/Handler/SignEngine/Pkcs12HandlerTest.php

It falls in category 1 of the suggested order: the test builds the real IL10N and asserts 5 translated messages, coming from three behaviors:

behavior	throw site	asserted message
no certificate stored for the signer	`SignEngineHandler::getPfxOfCurrentSigner()` (2 sites, code `400`)	Password to sign not defined. Create a password to sign.
the home folder refuses writing/deleting the PFX	`SignEngineHandler::savePfx()` / `deletePfx()`	You do not have permission for this action.
validating a file that carries no signature	`Pkcs12Handler::getSignatureValidation()`	Unsigned file.

Two things I would like to confirm before writing code:

Every class in lib/Exception/ today extends \Exception, not LibresignException. So a new typed exception here has to extend LibresignException, otherwise the existing catch (LibresignException) sites and the API payload would change behavior — which this issue asks us not to do. Is that the direction you want (for example PasswordToSignNotDefinedException extends LibresignException), or would you rather keep the single class and assert semantic codes kept next to the class that raises them, as in refactor(exception): keep the semantic error codes next to the classes that raise them #8196?
You do not have permission for this action. is thrown from six places: two here, two in ValidateHelper and two in SignFileService. A shared exception applied to only two of them would leave the concept half-converted, so I would leave that message out of this PR and do it in a dedicated one covering all six. Does that work for you?

A note about the sibling file, tests/php/Unit/Handler/SignEngine/JSignPdf/JSignPdfHandlerTest.php: it also asserts a message, but that one is not translated — it is the TSA diagnosis text built in checkTsaError(), where the wording that reaches the administrator is the behavior under test (#8274). Asserting a class or a code there would stop testing what that change was about, so I plan to leave it as it is unless you prefer otherwise.

Thanks, André. I think your questions show that we should define the error model a little better before we continue converting more tests.

Given how LibresignException is already used in LibreSign, I think it should remain the base for errors that are expected to cross the application boundary and be converted into API/frontend responses by InjectionMiddleware.

Because of that, when a typed exception represents a public LibreSign error, I think it should extend LibresignException, not \Exception.

For the "Password to sign not defined" case, for example, something like PasswordToSignNotDefinedException extends LibresignException seems reasonable. The test could then assert the exception type instead of the translated message.

At the same time, I think we should be careful with semantic error codes.

Today Exception::getCode() is used for two different things:

in older places, values like 400, 404 or 422 are HTTP status codes;
in newer places, values like 4010, 4220, 4221 or 5030 are semantic error codes.

This creates an ambiguity because InjectionMiddleware still uses getCode() directly as the HTTP response status.

Maybe it would be better to separate these two concepts explicitly in LibresignException.

For example, LibresignException could have an HTTP status property and a method like getHttpStatus(). Then:

the exception type can identify the application/domain error;
Exception::getCode() can be used only as a semantic code when a stable machine-readable code is really needed;
getHttpStatus() can be used only for the HTTP response;
the message can remain human-readable and translated.

I would prefer not to calculate the HTTP status automatically from codes such as 4220 -> 422. It would work technically, but it would still make the semantic code depend on the HTTP transport. If the HTTP status changes later, we should not need to change the identity of the error too.

Something like this would make the separation explicit:

throw new LibresignException(
    message: $this->l10n->t('Unable to process the file as a PDF document: %s', [$fileName]),
    code: PdfParser::CODE_INVALID_PDF,
    httpStatus: Http::STATUS_UNPROCESSABLE_ENTITY,
);

Then InjectionMiddleware would use getHttpStatus() instead of getCode().

This probably needs a small migration, because some current LibresignException calls still use the second argument as an HTTP status. I would not try to change everything in one PR, but I think it is worth defining the direction first so we do not add more mixed cases.

For the convention in this issue, my current preference would be:

prefer an existing typed exception when it already represents the error well;
create a typed exception when the error is an important and reusable application condition;
public typed exceptions should extend LibresignException;
use a semantic error code only when the frontend, API, SDK or another caller really needs to identify the error programmatically;
keep the HTTP status separate from the semantic code;
avoid testing translated messages when the text itself is not the behavior;
keep message assertions when the exact diagnostic text is really what the test is checking.

So for your current PR, I agree with leaving the permission error for a separate change covering all related occurrences. I also agree with keeping the TSA message assertion, because in that case the diagnostic text itself is part of the behavior being tested.

I also think this convention is important enough to document after we agree on it. My suggestion would be to add a small Architecture section to the developer documentation, with an Error handling page covering internal exceptions, public LibresignException errors, typed exceptions, semantic codes, HTTP statuses, translated messages and testing.

This would also fit the direction we have been discussing for AGENTS.md: instead of growing it with every LibreSign-specific rule, it can become a short gateway to the canonical developer documentation. An agent should be able to start from AGENTS.md, find the relevant project convention quickly, and then read the detailed rule in docs.libresign.coop.

For example, AGENTS.md could eventually point to the error handling documentation instead of duplicating the complete convention there. This should help us keep the agent instructions useful without making that file very large or having the same rule maintained in two places.

The API/OpenAPI documentation could then document concrete semantic error codes only when they are really part of the public endpoint contract.

What do you think about this direction? In particular, I would like your opinion on separating the HTTP status into getHttpStatus() before we continue adding more semantic codes.

Yes, I agree with separating the HTTP status from the semantic code, and after mapping the throw sites to answer you I think the case is stronger than the 400-vs-4010 ambiguity alone.

getCode() is carrying four different things today, and the two that have a real runtime consumer are not the ones we would expect.

1. HTTP status

422, 404, 400, 403, 500, Http::STATUS_*… read by InjectionMiddleware::getStatusCodeFromException() (lib/Middleware/InjectionMiddleware.php:481), which falls back to 422 when the code is 0.

2. Semantic codes

PdfParser::CODE_INVALID_PDF (4220), PdfValidator::CODE_DOCMDP_RESTRICTED (4221), AbstractIdentifyMethod::CODE_INVALID_TOKEN (4010), TwofactorGatewayService::CODE_NOT_ENABLED (5030).

I grepped for consumers, and none of the four reaches a client today. Their only readers are four expectExceptionCode() assertions in unit tests. Every escape route drops the code before the response is built:

code	route	what happens to the code
4220 / 4221	`FileController::save()` → `FileService::getNodeFromData()`	`catch (LibresignException)` returns `['message' => …]` with a hardcoded `STATUS_UNPROCESSABLE_ENTITY` (`FileController.php:592`)
4010	`SignFileController::sign()` → `AbstractIdentifyMethod::throwIfInvalidToken()`	`catch (\Throwable)` → `SigningErrorHandler`, hardcoded 422 (`SignFileController.php:184`)
5030	`TwofactorGatewayService::resolveIntegrationService()`	same catch

So by the rule you proposed — a semantic code only when a caller really needs to identify the error programmatically — these four do not qualify yet. They are good candidates to become typed exceptions carrying no code at all.

3. `JSActions` codes — this one is live

ValidateHelper::canSignWithIdentificationDocumentStatus() (ValidateHelper.php:910) throws with JSActions::ACTION_SIGN_ID_DOC, which is 2750. It travels through the catch (\Throwable) in SignFileController::sign() into SigningErrorHandler::handleLibresignException(), which copies getCode() into errors[].code. The frontend reads it back:

// src/services/SigningRequirementValidator.ts:104
const hasError = errors.some(error => error.code === ACTION_CODES.SIGN_ID_DOC)

with SIGN_ID_DOC: 2750 in src/helpers/ActionMapping.ts:36.

And the mirror case is exactly the pair I claimed in this issue. SignEngineHandler::getPfxOfCurrentSigner() throws Password to sign not defined with code 400 (lines 206 and 211), and SigningErrorHandler does:

$action = $code === 400
    ? JSActions::ACTION_CREATE_SIGNATURE_PASSWORD
    : JSActions::ACTION_DO_NOTHING;

That 400 is not an HTTP status in practice — it is the marker that drives the "create a signing password" screen (ACTION_CODES.CREATE_SIGNATURE_PASSWORD → CreatePassword, ActionMapping.ts:53). A refactor that reads those two 400s as "obviously the HTTP status" and moves them into httpStatus: would silently break that screen, and no test would catch it, because Pkcs12HandlerTest asserts the message and not the code.

This is the part I would not have noticed without your question, and it changes how I would write the PR I claimed.

4. A generic sentinel: `1`

22 of the call sites pass 1. Examples: AccountService.php:95, MailService.php:86, SignFileService.php:341, SignFileController.php:313. It is not an HTTP status and not semantic — it reads like "some error happened".

At least one of them escapes to the middleware. SignFileController::requestCodeBySignerUuid() throws it from inside its own catch, so nothing else catches it, and getStatusCodeFromException() returns 1 as the statusCode of the DataResponse. I have not checked what the HTTP layer finally emits for a status of 1, but it is worth checking — it may be worth treating separately from the migration.

Counting the migration

301 new LibresignException( in lib/, 80 of which pass a second argument:

second argument	count
`1`	22
explicit HTTP status (`422`, `404`, `400`, `500`, `Http::STATUS_*`, …)	51
`self::CODE_*` (semantic)	4
`JSActions::ACTION_SIGN_ID_DOC`	1
`0`	1

The mechanical part is small. What is not mechanical is that each of the 80 needs a decision about which of the four meanings it carries.

On the constructor

LibresignException declares no constructor today, so it inherits \Exception::__construct(string $message, int $code, ?Throwable $previous). To keep step one a pure refactor with no behavior change, I would put httpStatus after $previous, nullable, and have getHttpStatus() reproduce exactly what the middleware does today when it was not given:

public function __construct(
    string $message = '',
    int $code = 0,
    ?\Throwable $previous = null,
    private ?int $httpStatus = null,
) {
    parent::__construct($message, $code, $previous);
}

public function getHttpStatus(): int {
    if ($this->httpStatus !== null) {
        return $this->httpStatus;
    }
    // Legacy: getCode() used to be the HTTP status. Removed once every
    // call site passes httpStatus explicitly.
    return $this->getCode() === 0
        ? Http::STATUS_UNPROCESSABLE_ENTITY
        : $this->getCode();
}

InjectionMiddleware then calls getHttpStatus() for LibresignException and keeps its current logic for anything else (afterException also handles non-LibreSign exceptions on the HTML branch). Every existing call site keeps compiling and every response keeps the status it has today, so the first PR carries no behavior change at all — the legacy branch is what makes it safe, and deleting it is the last step rather than the first.

Suggested order

httpStatus + getHttpStatus() with the legacy fallback, InjectionMiddleware switched over. No behavior change.
Migrate the 51 explicit HTTP statuses to httpStatus:, in small groups per area.
Decide about the 22 occurrences of 1 — most of them look like they simply want the 422 default.
SigningErrorHandler and ACTION_SIGN_ID_DOC: these are a frontend contract, not a transport concern. I would document the current behavior first and convert it deliberately, not as a side effect of step 2. Until then, getPfxOfCurrentSigner()'s 400 must stay in getCode().
Only then remove the legacy branch, and revisit whether the four CODE_* constants still earn their existence.

Documentation

I agree with the Architecture section and the Error handling page, and with AGENTS.md becoming a short gateway instead of a second copy of the convention — the duplication is the part that actually goes stale.

One thing I would add to that page: a table of what errors[].code means on the signing endpoints today. Sections 3 and 4 above are not guessable from the code, and I think a newcomer converting a test is exactly the person most likely to break them.

About the PR I claimed

The convention you settled answers it: PasswordToSignNotDefinedException extends LibresignException, and the test asserts the type instead of the message. But because of the 400 → ACTION_CREATE_SIGNATURE_PASSWORD mapping, the new class has to keep passing 400 as its code until step 4 above, otherwise the signer loses the "create a signing password" screen.

The mapping itself is already covered — SigningErrorHandlerTest has a code 400 triggers password creation action dataset. What is not covered is the link: nothing connects the literal 400 in SignEngineHandler to that dataset, so the two can drift apart without a red test. I would close that in the same PR by asserting the code at the throw site as well, so the new typed exception documents why the 400 is there.

Does that work for you, or would you rather I wait for step 1 to land before touching the pair?

Friendly ping on this one, no rush. The open point from my last comment is whether to give LibresignException its own HTTP status (getHttpStatus() with the legacy fallback to getCode()) before converting more tests.

If it helps to move forward, I can send step 1 alone as a small PR: the constructor argument, getHttpStatus() and InjectionMiddleware reading it, with no behavior change. The rest of the discussion could then happen on code.

Hi! I’d like to take the CRL lock pair: CrlServiceTest asserts “CRL refresh/generation is already in progress” by message, and CrlRevocationChecker also detects that case with str_contains($e->getMessage(), 'already in progress'). I plan to add a typed CrlGenerationInProgressException (extending LibresignException, same message), use instanceof in the checker, and update the tests to assert the class. Small, single PR.

refactor(crl): use typed exception for CRL lock in progress

@maia-andre

Hi @maia-andre and @matheusa1,

Thanks for your work on this issue.

@matheusa1, the CRL lock exception work was completed in #9102. Thanks for implementing it! We can consider this part done.

@maia-andre, I reviewed your proposal, and I agree with separating the HTTP status from the semantic exception code.

Please go ahead with step 1 as a small PR:

Add the optional httpStatus constructor argument and getHttpStatus() method to LibresignException.
Keep the legacy fallback for backward compatibility.
Update InjectionMiddleware to use the new method for LibresignException.
Add tests covering legacy behavior, explicit HTTP statuses, and unchanged response bodies.

Please keep existing exception codes and frontend actions unchanged. We will handle their migration separately.

For the signing password exception you mentioned earlier, you can continue with that work after step 1. Please preserve the current code 400 behavior that opens the password creation screen.

Issue #8117 will remain open to track the remaining work. It does not need one contributor to implement everything. Other contributors can take small, independent parts, with coordination here to avoid duplicate work.

We will also update the issue description to reflect the agreed convention and the work already completed.

Thanks again for your contributions!

Hi! I'd like to take the identification-document pair:

lib/Service/UuidResolverService.php
tests/php/Unit/Service/UuidResolverServiceTest.php

Like #9102, resolveUuidForUser() decides whether to rethrow two errors by
comparing $e->getMessage(). I plan to add IdDocAccessDeniedException and
NotAnIdDocException (extending LibresignException, same messages, code 0),
replace the comparison with a typed catch, and make the tests assert the
class instead of the message.

No HTTP status or frontend code involved, so it doesn't depend on step 1.

Navigation Menu

Repository navigation

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

Current state

What should be decided first

How to contribute

Effort

1. HTTP status

2. Semantic codes

3. `JSActions` codes — this one is live

4. A generic sentinel: `1`

Counting the migration

On the constructor

Suggested order

Documentation

About the PR I claimed

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

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

Description

Current state

What should be decided first

How to contribute

Effort

Activity

ajayraho commented on Aug 31, 2026

vitormattos commented on Aug 31, 2026

ajayraho commented on Aug 31, 2026

vitormattos commented on Aug 31, 2026

ajayraho commented on Aug 31, 2026

vitormattos commented on Aug 31, 2026

vitormattos commented on Sep 1, 2026

maia-andre commented on Sep 2, 2026

maia-andre commented on Sep 6, 2026

vitormattos commented on Sep 6, 2026

maia-andre commented on Sep 7, 2026

1. HTTP status

2. Semantic codes

3. JSActions codes — this one is live

4. A generic sentinel: 1

Counting the migration

On the constructor

Suggested order

Documentation

About the PR I claimed

maia-andre commented on Oct 2, 2026

matheusa1 commented on Oct 17, 2026

vitormattos commented on Oct 24, 2026

AdaaWu commented on Oct 24, 2026

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions

3. `JSActions` codes — this one is live

4. A generic sentinel: `1`