Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/base64-docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ jobs:
with:
php-version: ${{ matrix.php }}

# The autoloader the check runs under is the one a consumer gets, src/Upload/i18n.php
# included: an unresolved `files` entry reports __() as undefined from inside File.php.
# The autoloader the check runs under is the one a consumer gets
- name: Install the library
run: composer install --no-dev --no-interaction --no-progress

Expand Down
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ A security release. New protections are on by default and will refuse some uploa
* **`Upload\Exception`'s constructor takes the error code and the message's values before `$code` and `$previous`.** The signature is now `__construct(string $message, ?FileInfoInterface $fileInfo = null, string $errorCode = ErrorCode::NONE, array $messageArgs = [], int $code = 0, ?Throwable $previous = null)`. **A validation of your own passing `$code` or `$previous` positionally is passing them to the wrong parameters.** `new Exception($message, $fileInfo)` is unaffected
* **`File::recordError()` takes a message id, its values, an error code and a filename** rather than a finished line: `recordError(string $messageId, array $args = [], string $errorCode = ErrorCode::NONE, ?string $filename = null)`. A `File` subclass that records an error of its own passes the message and the filename separately, and the `"filename: message"` composition happens in one place. `recordError('Rejected by the scanner')` still does what it did
* **`File::$errors` and `File::$constructorErrors` are gone**, replaced by a `private $errorDetails` holding each error as its parts. A subclass appending to `$errors` directly was the one route around the sanitizing guarantee `getErrors()` carries, and it is what would have left `getErrorDetails()` with holes. **Use `recordError()`**, which is still `protected`, and `getErrors()` to read
* **`File::$errorCodeMessages` is now the method `File::getUploadErrorMessages()`.** A PHP 7.3 constant expression cannot call a function, so an array of literals is all a property could hold and no extractor can see one. Each value is marked with `__()` instead. A subclass overriding the wording overrides the method
* **`File::$errorCodeMessages` is now the method `File::getUploadErrorMessages()`.** A PHP 7.3 constant expression cannot call a function, so an array of literals is all a property could hold and no extractor can see one. Each value is marked with `Translation::__()` instead. A subclass overriding the wording overrides the method
* **`'5MB'` now means 5 MiB, not 5 bytes.** `File::humanReadableToBytes()` reads a trailing `B`, where `substr($input, -1)` saw only the `B`. **Every `KB`/`MB`/`GB` bound you pass becomes much larger.** Bounds without the trailing `B` are unchanged
* **An unrecognized unit throws** instead of being read as bytes. `new Size('1T')` was a one-byte bound that rejected every upload while reading as a generous one. **Check any bound whose unit isn't `B`, `K`, `M` or `G`**
* **`FileInfo::getMd5()` is removed** from `FileInfo` and `FileInfoInterface`. `getHash('md5')` returns the same digest
Expand All @@ -65,7 +65,7 @@ A security release. New protections are on by default and will refuse some uploa
## New Features

* **Error messages can be translated, and no translations ship.** `Translation::setTranslator(callable)` installs a `function (string $text, string $domain): string` that `getErrors()` is looked up through. With none installed every message is the English it has always been, because the English string *is* the message id. Values are interpolated after the lookup, so a `%` in a filename or a size bound is never read as a `printf` conversion. A translator that throws, returns a non-string, returns an empty string, or returns a template whose placeholders do not match is ignored in favour of English, so a broken catalogue cannot turn a rejected upload into a fatal error and a partly-translated one cannot blank a message. Called at render time, so a locale switched mid-request is the one used
* **`GravityPdf\Upload\__()` marks each translatable string** where it is written — gettext's `N_()` idiom under a familiar name, `__(string $text, string $domain = Translation::DOMAIN)`. It returns `$text` and never translates; the lookup happens once, when `getErrors()` is read, which keeps `Exception::getMessage()` in English for your log and the raw msgid in `getErrorDetails()`. `xgettext --keyword=__` finds every call. `$domain` is for your extractor, not the runtime, which always looks up under `Translation::DOMAIN`. Namespaced, so it coexists with WordPress's global `__()` — but a file outside `GravityPdf\Upload` must import it or call it fully qualified, since an unqualified call with neither falls back to the global one
* **`Translation::__()` marks each translatable string** where it is written — gettext's `N_()` idiom under a familiar name, `__(string $text, string $domain = Translation::DOMAIN)`. It returns `$text` and never translates; the lookup happens once, when `getErrors()` is read, which keeps `Exception::getMessage()` in English for your log and the raw msgid in `getErrorDetails()`. `xgettext --keyword=__` finds every call: the keyword matches the trailing identifier, so `Translation::__()` extracts exactly as a bare `__()` does. `$domain` is for your extractor, not the runtime, which always looks up under `Translation::DOMAIN`. A static method rather than a function, so it loads with the class — a bare function needs Composer's `files` autoload entry, which an autoloader that only indexes classes never runs, and this library ships no such entry
* **`i18n/upload.pot`**, the catalogue of every string that gets looked up, with translator comments on the ones that take values. A template only — no `.po` or `.mo` files. Rebuilt by `composer i18n:pot` and pinned by a workflow that fails on a diff, so a reworded message cannot orphan a consumer's translation of it. `Storage\FileSystem`'s messages are absent: they name the destination and are written for your log, not for whoever submitted the file
* **`File::getErrorDetails(): array`** returns the same failures as their parts: `code`, the untranslated `message_id` and its `args`, the sanitized `filename` or `null`, and the finished `message`. Use it to branch on a failure, or to word one yourself. `getErrors()` is its `message` column
* **`ErrorCode`** names every failure this library reports, as string constants that stay stable while wording does not: the `UPLOAD_ERR_*` outcomes, the collection's own, the shipped validations' and storage's. `Exception::getErrorCode()` returns one, alongside `getMessageId()` and `getMessageArgs()`
Expand Down
16 changes: 10 additions & 6 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ The `i18n` workflow regenerates the catalogue and fails on a diff, which is what

`tools/psr7-readme/` holds a second scratch manifest, for the same reason `tools/phpstan/` does: `docs/psr7.md` documents a PSR-7 bridge as caller code, and `verify.php` runs those snippets — **read out of the Markdown at run time, never copied** — against nyholm/psr7 and guzzlehttp/psr7. Rename a documented class or drop an example and the extraction fails by name. The library still requires nothing but `ext-fileinfo`; the check has its own `composer.json`, its own composer script and its own workflow. `verify.php` is covered by `lint` and `check-syntax` — it is the documented code's only safety net, so it does not get to sit outside the checks everything else passes.

`docs/base64-uploads.md` is the same arrangement for a file posted as base64 in a JSON body, run by `tools/base64-docs/verify.php`. That directory has no manifest: a base64 payload is a string in the request body, so there is no third-party implementation to run the bridge against, and the check loads the library through the root `vendor/autoload.php`the autoloader a consumer gets, `src/Upload/i18n.php` included. The page declares one snippet each for the bridge and for validating and storing what it returns.
`docs/base64-uploads.md` is the same arrangement for a file posted as base64 in a JSON body, run by `tools/base64-docs/verify.php`. That directory has no manifest: a base64 payload is a string in the request body, so there is no third-party implementation to run the bridge against, and the check loads the library through the root `vendor/autoload.php`, the autoloader a consumer gets. The page declares one snippet each for the bridge and for validating and storing what it returns.

The `phpunit` workflow carries a second job, `cross-file-system`, which mounts a tmpfs and points `UPLOAD_TEST_OTHER_FS` at it so `FileSystemTest::testStoresAFileFromAnotherFileSystem()` runs against two real file systems rather than skipping. It runs the **whole suite** with `--fail-on-skipped` rather than filtering to that test: a `--filter` matching nothing exits 0 with "No tests executed!", so it needed a second guard to prove it had run, and renaming the test was enough to trigger exactly that. That test is the suite's only `markTestSkipped()`, which is what makes one guard sufficient — keep it that way, or the job goes quiet. Tests that cannot run in a given configuration are excluded by group instead: `@group mbstring` marks the ones that need the UTF-8 repair, so the `no-mbstring` job drops them with `--exclude-group` rather than adding a second skip, and `@group posix` marks the ones that need `symlink()`, `chmod()` or `umask()` to mean something, or a name NTFS refuses outright, which the `windows` job drops the same way. Set `UPLOAD_TEST_OTHER_FS` to run it locally: on macOS, `hdiutil attach -nomount ram://8192` then `diskutil erasevolume HFS+ UPLOADTMP <disk>`.

Expand Down Expand Up @@ -95,21 +95,25 @@ Four optional callbacks (`beforeValidate`, `afterValidate`, `beforeUpload`, `aft

**`render()` rejects a mismatched translation, and only where there are values to fill.** Conversions that do not match the source's would interpolate wrongly — but with no values `interpolate()` hands the template back whole and nothing can go wrong, so checking anyway only rejected good translations. A space is a legal `printf` flag, so `50% of your quota is used` carries one conversion by PHP's own grammar (`vsprintf()` raises on it) while a German translation carrying no `%` counts none. Before the interpolation rather than in it, because that is the last point where the English is still in hand.

**The call sites mark, they do not translate.** `GravityPdf\Upload\__(string $text, string $domain = Translation::DOMAIN)` is gettext's `N_()` idiom under a familiar name and shape: it returns its argument, and the lookup happens once, when `getErrors()` is read. That keeps `getMessage()` English, keeps an untranslated msgid in `getErrorDetails()`, and takes the locale when the message is read rather than when the file was rejected. A `__()` that translates in place was rejected for those three reasons, despite the shape being the familiar one.
**The call sites mark, they do not translate.** `Translation::__(string $text, string $domain = self::DOMAIN)` is gettext's `N_()` idiom under a familiar name and shape: it returns its argument, and the lookup happens once, when `getErrors()` is read. That keeps `getMessage()` English, keeps an untranslated msgid in `getErrorDetails()`, and takes the locale when the message is read rather than when the file was rejected. A `__()` that translates in place was rejected for those three reasons, despite the shape being the familiar one.

**Sharing the name with WordPress's global `__()` is what that familiarity costs.** PHP resolves an unqualified call against the global namespace when the current one has no match, so a file under `GravityPdf\Upload\Validation` that forgets `use function GravityPdf\Upload\__;` reaches WordPress's function instead — silently, translating at the throw. `File` and `Translation` declare `GravityPdf\Upload` and cannot take that path; the four validators can. `I18nTest::testEveryCallerCanReachTheMarker()` reads every file under `src/` for it, and is the only guard. It runs as **one** test over the corpus and asserts it found at least one marked string: per-file, it could only assert about a file that marks something, so renaming the marker made every case vacuous while the suite stayed green. The marker has been renamed twice. Outside WordPress the same mistake is a plain fatal, which is why `testNoGlobalMarkerCanMaskAMissingImport()` pins that `tests/bootstrap.php` never grows a global `__()` stub — do not add one. A fully qualified `\GravityPdf\Upload\__()` is immune to the fallback and extracts identically.
**The marker is a static method because a function is unreachable under a classmap.** It was `GravityPdf\Upload\__()`, declared in `src/Upload/i18n.php` and loaded by Composer's `files` autoload. That entry is Composer's own: an autoloader that indexes classes and nothing else — a classmap over php-scoper output, which is how a WordPress plugin usually consumes this library — never runs it, so `__()` was undefined on every error path while uploads succeeded (issue #42). Do not put a bare function back, and do not add a `files` entry; `I18nTest::testTheMarkerTravelsWithTheClass()` refuses both.

Moving it onto `Translation` also ends the namespace fallback. An unqualified **function** call resolves against the global namespace when the current one has no match, so a validator forgetting `use function GravityPdf\Upload\__;` reached WordPress's `__()` and translated at the throw. A **class** name has no such fallback: a validator forgetting `use GravityPdf\Upload\Translation;` fatals at the call instead. That is still only reached on an error path, so `I18nTest::testEveryCallerCanReachTheMarker()` reads every file under `src/` for the import rather than waiting for a rejected upload. It runs as **one** test over the corpus and asserts it found at least one marked string: per-file, it could only assert about a file that marks something, so renaming the marker made every case vacuous while the suite stayed green. The marker has been renamed twice.

Extraction needs no configuration for the move. `xgettext` matches the keyword against the trailing identifier and ignores the class prefix, so `-k__:1` reads `Translation::__()`, `\GravityPdf\Upload\Translation::__()` and an aliased `T::__()` alike. `i18n/upload.pot` regenerating byte-identical across the move is the proof.

**The marker's `$domain` is extractor-facing.** It is discarded, and `render()` looks every message up under `Translation::DOMAIN` regardless, so marking a string of your own under your own domain describes it to your catalogue tooling and redirects nothing. It defaults to `Translation::DOMAIN` for the same reason WordPress's `__()` defaults to `'default'`. **The calls in `src` do not pass it** — every string here belongs to that domain and the default already says so, where a literal at twenty call sites is twenty places to drift. The extraction keyword must stay `-k__:1`: `-k__:1,2` reads the second argument as a plural form and extracts **nothing at all**.

**Only what `File` renders is translated.** `Exception::getMessage()` is always English, composed in the constructor from the message id and its values. `getMessage()` is `final` and `__toString()` reads the internal property rather than the getter, so a lazily translated message would be one string to a `catch` and another to an uncaught-exception handler; an exception is also the half of this library written for a log, which it stops being once translated. That covers all of `Storage\FileSystem`, whose messages name the destination and would be an existence oracle if shown, and `File`'s own `'File validation failed'`/`'There are no files to upload'`. Those are simply not marked, so a string a translator cannot affect is never offered to them — the narrowing is the marker's doing rather than the generator's, and adding a keyword cannot widen it by accident. `CatalogueTest` pins both halves: the marked strings are in the catalogue, storage's are not.

**`Validation\Size` has a message id per unit, not a unit per value.** The bound is scaled to the largest of B/KB/MB/GB it reaches and the unit sits inside the string, because a value is interpolated after the lookup and a translator never sees it — French writes `Mo`, so `MB` cannot be a value. `getTooLargeMessages()`/`getTooSmallMessages()` hold the eight, as methods for the same reason `getUploadErrorMessages()` is one. `scale()` rounds a maximum down and a minimum up so the size named is always one the file would pass at: 5,000,000 bytes reported as `4.8 MB` would name a size still rejected. Its decimal separator is a `.`, because picking one needs a locale this library does not take — **`scale()` is `protected` and called through `static::` for that reason**, and is the seam for it. An override owns the number and the unit together, which a number-formatter hook on `Translation` could not: the unit is chosen here, and by the time a value is interpolated the message id naming it has already been picked. It only ever comes up for a limit configured in decimal bytes; `'5M'`, `'500K'` and any binary count divide exactly into the 1024-based ladder and render whole.

**`getUploadErrorMessages()` is a method because a PHP 7.3 constant expression cannot call `__()`.** The seven `UPLOAD_ERR_*` strings were a `protected static` array of literals, which no extractor can see; a property could hold nothing else, so they moved to a method for the same reason `FileSystem::getDefaultBlockedExtensions()` is one. A subclass overriding the wording overrides the method, and its strings are its own to extract.
**`getUploadErrorMessages()` is a method because a PHP 7.3 constant expression cannot call `Translation::__()`.** The seven `UPLOAD_ERR_*` strings were a `protected static` array of literals, which no extractor can see; a property could hold nothing else, so they moved to a method for the same reason `FileSystem::getDefaultBlockedExtensions()` is one. A subclass overriding the wording overrides the method, and its strings are its own to extract.

**A translator comment must sit immediately above the `__()` call**, inside the argument list, not above the `throw`. `xgettext` attaches a comment to the line the string is on; move it up two lines to sit above `throw new Exception(` and it is silently dropped from the catalogue.
**A translator comment must sit immediately above the `Translation::__()` call**, inside the argument list, not above the `throw`. `xgettext` attaches a comment to the line the string is on; move it up two lines to sit above `throw new Exception(` and it is silently dropped from the catalogue.

**`src/Upload/i18n.php` is loaded by Composer's `files` autoload**, the only entry in it. One consequence: a tool installing this library from a path repository — `tools/psr7-readme/` — only sees a change to the autoload configuration when the package is re-resolved, so that script runs `composer update` rather than `install`. A stale vendor directory reports `__()` as undefined, from inside `File.php`.
**`composer.json` declares PSR-4 and nothing else.** The tools installing this library from a path repository — `tools/psr7-readme/`, `tools/translator-readme/` — run `composer update` rather than `install`, because a lock records the path package's autoload configuration and `install` replays the one it was written against.

**`ErrorCode` is the stable identifier; the message is not.** Wording is translated and edited between releases, so a caller branching on it breaks quietly — `getErrorDetails()` and `Exception::getErrorCode()` carry a code for that instead. A code is not a message id: the id travels with the catalogue and changes when the wording does. A validator of your own that throws without naming one is recorded as `VALIDATION_REJECTED` rather than as nothing, so every entry in `getErrorDetails()` has something to branch on.

Expand Down
Loading
Loading