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: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,13 @@ A security release. New protections are on by default and will refuse some uploa

* **A developer error no longer throws the type you catch for a failed upload.** `File::upload()` throws `\LogicException` with no validations configured, and `FileInfo::getHash()` throws `\InvalidArgumentException` for an unsupported algorithm. **Code catching `\GravityPdf\Upload\Exception` around `upload()` needs `\LogicException` too**; `catch (\Exception $e)` is unaffected
* **`upload()` no longer dispatches through the public `isValid()`.** Both entry points validate through a private method that reports which files passed, so `uploadValid()` doesn't validate twice and a validator with a side effect sees each file once. `isValid()` is unchanged when you call it yourself, but **a subclass that overrode it to add a check no longer has that check run by `upload()`**. Move it into a `ValidationInterface`, which both entry points honour
* **`File::isValid()`, `File::upload()` and `File::uploadValid()` are `final`**, so a subclass carrying one of them is a fatal error when it loads rather than a check that silently stopped running. The three share the reset-then-validate sequence, the re-entrancy lock and the count that decides which files passed, and a partial override breaks it. A check of your own goes in a `ValidationInterface`; work either side of the storing goes in the `beforeUpload`/`afterUpload` callbacks
* **`File::__call()` throws `\BadMethodCallException`** for a method the underlying file object does not have, so a typo isn't reported to the end user as a failed upload. `FileInfo::createFromFactory()` throws `\LogicException` where it threw `\RuntimeException`
* **`FileInfoInterface`'s three setters no longer declare a return type.** They declared `: FileInfo`, so an implementation that didn't extend `FileInfo` compiled and then raised a `TypeError` on the first setter call: it couldn't return `$this`, and couldn't narrow the return type either, since covariant returns need PHP 7.4 and this library supports 7.3. **A custom `FileInfoInterface` is only now implementable.** An existing subclass declaring `: FileInfo` still satisfies the interface
* **`FileSystem::blockExtensions()` takes a required, non-empty list.** `null` meant the full deny-list and `[]` meant none, so one expression turned a security control off depending on what a config key held. `[]` now throws `InvalidArgumentException`, and `allowAnyExtension()` is the only way to empty the list. Pass `getDefaultBlockedExtensions()` for the old no-argument meaning
* **`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::$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. Reading, writing or `isset()`-ing either old name throws `\LogicException` naming both methods, as does reaching for `$errorDetails`, `$constructorErrorDetails` or anything else `File` keeps `private`: an append to a property that no longer exists creates a dynamic one nothing reads, and before PHP 8.2 it raises nothing at all. A subclass that *declares* one of those names, or the removed `protected static $errorCodeMessages`, is refused when it is constructed — a declared property is in scope, so no magic method can see it, and `empty($this->errors)` would have answered `true` for a collection that rejected every file
* **`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`**
Expand Down
6 changes: 3 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,17 @@ The `windows` job is a gate. It found two bugs on its first outing, both on the

`Validation\FileType` replaces `Extension` + `Mimetype` used side by side: those two check independent allow-lists, so a file passes both while the two answers describe different formats. `FileType` keys media types by extension, so each `allow()` call describes one format. Both older classes are `@deprecated` as of 4.0.0 but still work, with no runtime notice.

**`upload()` requires something to validate against.** It throws `\LogicException` when `$validations` is empty, unless `File::allowUnvalidatedUploads()` was called. The type is the point: `Upload\Exception` is what a caller catches around `upload()` to handle a rejected file, and a misconfigured object must not land in that branch. The check sits in `upload()` rather than `isValid()` for the same reason — it is a configuration error for the developer, not a per-file failure to show an end user through `getErrors()`. `upload()` also throws when the collection is empty, since every validation passes vacuously against nothing. Both guards are shared with `uploadValid()`, which is `upload()` without the all-or-nothing guarantee: it stores the files that passed and leaves the rest in `getErrors()`. The two differ only in what they do with the result of the shared `prepareUpload()` — do not let them grow separate prologues. `store()` keys `$uploadedFiles` by **collection offset** rather than appending: a partial batch renumbered into a list pairs a stored file with a rejected one's metadata in any caller that zips the two by index.
**`upload()` requires something to validate against.** It throws `\LogicException` when `$validations` is empty, unless `File::allowUnvalidatedUploads()` was called. The type is the point: `Upload\Exception` is what a caller catches around `upload()` to handle a rejected file, and a misconfigured object must not land in that branch. The check sits in `upload()` rather than `isValid()` for the same reason — it is a configuration error for the developer, not a per-file failure to show an end user through `getErrors()`. `upload()` also throws when the collection is empty, since every validation passes vacuously against nothing. Both guards are shared with `uploadValid()`, which is `upload()` without the all-or-nothing guarantee: it stores the files that passed and leaves the rest in `getErrors()`. The two differ only in what they do with the result of the shared `prepareUpload()` — do not let them grow separate prologues. All three entry points are **`final`**: they share the reset-then-validate sequence, the re-entrancy lock and the error count that decides which files passed, and 4.0.0 moved the validating into a `private` method, so an override of `isValid()` that used to run on every `upload()` now runs on none. `final` is what makes that a fatal at load rather than a check that quietly stopped running. A check of a caller's own belongs in a `ValidationInterface`; wrapping belongs in the callbacks. `store()` keys `$uploadedFiles` by **collection offset** rather than appending: a partial batch renumbered into a list pairs a stored file with a rejected one's metadata in any caller that zips the two by index.

**`FileList` is the same collection, built from files the caller supplies.** `new FileList($fileInfos, $storage, $failures)` skips the `$_FILES` reader entirely — it does not call `parent::__construct()`, because that method checks `file_uploads` and requires `$_FILES[$key]`, neither of which applies to a worker runtime or a PSR-7 bridge. Everything after construction is `File`'s, bar the two `ArrayAccess` writers it overrides: `offsetSet()` and `offsetUnset()` drop that offset's entry from `$sourceKeys`, so a caller's key can never outlive the file it named and `getSourceKeys()[$i]` cannot start describing something else. A malformed entry **throws `InvalidArgumentException`** rather than being recorded, which is the opposite of what the `$_FILES` path does with one, because this array is assembled by the developer rather than sent by a client — the same reasoning as `File::offsetSet()`. Both arguments' keys are discarded as offsets, since `$objects`, `getUploadedLocators()` and the `ArrayAccess<int, FileInfoInterface>` annotation are all offset-keyed; `$fileInfos`' keys are kept beside the collection and read back with `getSourceKeys()`, so a form field name survives without becoming an offset. Do not key the collection by them.

**`init()` is where a constructor invariant goes**, not `File::__construct()`. Two constructors and only one reads the superglobal, so an invariant added to the reader silently does not hold for `FileList` — the drift that produced the pre-4.0.0 filename bug. It is `protected` only because `FileList::__construct()` cannot reach a `private` method of its parent, and it is not an extension seam: it snapshots `$errorDetails` into `$constructorErrorDetails`, which is what `isValid()` resets to. Narrowing `$objects` or `$storage` to `private` is breaking for an in-repo class now. The two error lists already are `private`, as of 4.0.0, which is what makes `recordError()` the only route rather than merely the intended one — do not widen them back.
**`init()` is where a constructor invariant goes**, not `File::__construct()`. It holds one: `guardAgainstReplacedMembers()`, which refuses a subclass declaring a name 4.0.0 replaced. Two constructors and only one reads the superglobal, so an invariant added to the reader silently does not hold for `FileList` — the drift that produced the pre-4.0.0 filename bug. It is `protected` only because `FileList::__construct()` cannot reach a `private` method of its parent, and it is not an extension seam: it snapshots `$errorDetails` into `$constructorErrorDetails`, which is what `isValid()` resets to. Narrowing `$objects` or `$storage` to `private` is breaking for an in-repo class now. The two error lists already are `private`, as of 4.0.0, which is what makes `recordError()` the only route rather than merely the intended one — do not widen them back.

**`isUploadedFile()` is deliberately the caller's decision on the `FileList` path.** `runValidations()` rejects any file answering `false`, and on the `$_FILES` path that is PHP's SAPI assertion; nothing off that path can make it, so a `FileList` of plain `FileInfo` objects validates to nothing. That is correct rather than an obstacle — the caller overrides `isUploadedFile()` on a `FileInfo` subclass (the constructor is `final`, the class and the method are not) and asserts what their runtime can, and pairs it with `FileSystem::acceptFilesNotUploadedByPhp()` at the other end, without which storage refuses the file anyway. The library does not pretend to make an assertion it cannot make on someone else's input. This is the first thing the README section says, and it must stay that way.

**The constructor never trusts the shape of `$_FILES`.** A PSR-7 bridge or test harness can supply an entry that is not an array, or one missing `tmp_name`/`name`/`error`, or a multi-file entry whose keys are not parallel arrays. Every such shape is recorded as `'An uploaded file was sent in a format that cannot be read'` rather than warning or raising a `TypeError` — remote input must not warn.

**Validation errors accumulate; they don't abort.** `File::isValid()` runs every validation against every file and collects the failures, so `getErrors()` reports all of them at once. `upload()` throws only after the fact, with the generic message `'File validation failed'` — the detail is in `getErrors()`. An `Upload\Exception`'s message goes through `Filename::sanitizeForDisplay()` before it lands in `getErrors()`, sanitized there rather than in each validator because `getErrors()` is what carries the guarantee. A throw that never crosses a `File` boundary has no such chokepoint and sanitizes at the throw site instead, which is what `Storage\FileSystem`'s collision message does. **No shipped validator can reach that line with anything but configuration** — `Validation\FileType` prints an extension back only after it matches the configured allow-list, and the other three interpolate configuration alone. Do not re-document this as covering a shipped validator: it covers one of your own, and `FileList::describeKey()`, whose key is a field name the client chose. **`File::recordError()` is the only route into `$errorDetails`** — one `protected` method, and since 4.0.0 the only possible one, because the list it writes to is `private`. It records the message id, its values, an `ErrorCode` and a sanitized filename rather than a finished line; `getErrorDetails()` translates and composes, and `Filename::sanitizeForDisplay()` runs there. So the guarantee is structural rather than nine call sites each remembering, and it now covers the translation as well as the message — a catalogue is application-supplied text arriving on exactly the path the README says to render. The filename is sanitized at the call site instead, by `getSanitizedFilename()`/`Filename::sanitizeNameWithExtension()`, because those apply the naming rules and would report `user.avatar` as `user-avatar` if the renderer ran them over the whole line. The `'%s: %s'` join is deliberately not translatable; it is punctuation. `FileTest::testNothingElseAppendsToTheErrorList()` reads the source to keep the recorder the only writer. A validator throwing something other than `Upload\Exception` is absorbed too, with **both** its message and its class name dropped — a PHP runtime message can contain absolute paths, and a class name is the application's internal structure.
**Validation errors accumulate; they don't abort.** `File::isValid()` runs every validation against every file and collects the failures, so `getErrors()` reports all of them at once. `upload()` throws only after the fact, with the generic message `'File validation failed'` — the detail is in `getErrors()`. An `Upload\Exception`'s message goes through `Filename::sanitizeForDisplay()` before it lands in `getErrors()`, sanitized there rather than in each validator because `getErrors()` is what carries the guarantee. A throw that never crosses a `File` boundary has no such chokepoint and sanitizes at the throw site instead, which is what `Storage\FileSystem`'s collision message does. **No shipped validator can reach that line with anything but configuration** — `Validation\FileType` prints an extension back only after it matches the configured allow-list, and the other three interpolate configuration alone. Do not re-document this as covering a shipped validator: it covers one of your own, and `FileList::describeKey()`, whose key is a field name the client chose. **`File::recordError()` is the only route into `$errorDetails`** — one `protected` method, and since 4.0.0 the only possible one, because the list it writes to is `private`. It records the message id, its values, an `ErrorCode` and a sanitized filename rather than a finished line; `getErrorDetails()` translates and composes, and `Filename::sanitizeForDisplay()` runs there. So the guarantee is structural rather than nine call sites each remembering, and it now covers the translation as well as the message — a catalogue is application-supplied text arriving on exactly the path the README says to render. The filename is sanitized at the call site instead, by `getSanitizedFilename()`/`Filename::sanitizeNameWithExtension()`, because those apply the naming rules and would report `user.avatar` as `user-avatar` if the renderer ran them over the whole line. The `'%s: %s'` join is deliberately not translatable; it is punctuation. `FileTest::testNothingElseAppendsToTheErrorList()` reads the source to keep the recorder the only writer. `__get()` and `__set()` throw `LogicException` for `errors`, `constructorErrors`, `errorDetails` and `constructorErrorDetails`, so a subclass still using the names 3.x published is told rather than left writing to a dynamic property nothing reads — silently, on PHP 7.3 to 8.1. `__get()` is not redundant there: `$this->errors[] = $message`, which is how a 3.x subclass recorded a failure, is a read. They run in `File`'s scope, so the guard covers everything else the class declares `private` as well — `$running` above all, which a bare fallback would have let a subclass take from a callback. `__isset()` is on the same guard, since `empty($this->errors)` was how 3.x asked whether anything had failed and PHP would answer `true` for a collection that rejected every file. A name the class does not declare is created as before, but an **append** to one is not: that is a read, so PHP discards it with `Indirect modification of overloaded property`. **A magic method cannot see a property the subclass declares**, and a static never dispatches to one at all, so `init()` refuses a subclass declaring any of the five at construction — `File::REPLACED_MEMBERS` is that list, and `getUploadErrorMessages()` is why `errorCodeMessages` is on it. A validator throwing something other than `Upload\Exception` is absorbed too, with **both** its message and its class name dropped — a PHP runtime message can contain absolute paths, and a class name is the application's internal structure.

**`LogicException` is the exception to that.** It is re-thrown rather than absorbed, because PHP defines the type as a bug in the program. `FileInfo::getHash()` throws `InvalidArgumentException` for an unsupported algorithm precisely so a misspelling reaches the developer instead of the end user as a rejected upload. Absorb it and that guarantee is empty.

Expand Down
Loading
Loading