From 3a7c68a72375ba77e9e28a1c65c59021e240f1c4 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Tue, 25 Aug 2026 06:09:33 +1000 Subject: [PATCH] Make the seams that closed say so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isValid()` stopped being an extension seam in 4.0.0 without a word: `upload()` routes through the private `runValidations()`, so an override is never called, and the `protected $errors` it wrote to became `private $errorDetails`, so the append lands on a dynamic property nothing reads. A subclass loosening a check fails safe. One adding a check — a scan, a quota, a stricter name rule — has it bypassed on every upload, on a release whose headline is security. `final` on `isValid()`, `upload()` and `uploadValid()`, which is what PHP has to say "this is not a seam": the three share the reset-then-validate sequence, the re-entrancy lock and the error count that decides which files passed, so a partial override breaks one of them. `prepareUpload()` and `store()` are private either way, so an override could only ever have wrapped these, which is what the `beforeUpload`/`afterUpload` callbacks are for. The property is guarded on all four routes to it, since each one is silent on its own. `__set()` catches an assignment, `__get()` catches `$this->errors[] = $message` — an append is a read — and `__isset()` catches `empty($this->errors)`, which PHP would otherwise answer `true` for a collection that rejected every file. The magic methods run in `File`'s scope, so the guard covers the rest of what the class declares `private`, `$running` among it: without that arm a subclass assigning to the re-entrancy lock by name would have taken the real one. None of that can see a property the subclass declares, and a static never dispatches to a magic method at all, so `init()` refuses a subclass declaring any of the five names at construction — `$errorCodeMessages`, which became `getUploadErrorMessages()`, is reachable no other way. `UPGRADE.md` gets the two columns a migration is actually read for. The rows that did not move are half of it: `FileInfo::isUploadedFile()` and `getReservedWindowsNames()` still hold, `FileSystem::resolveFilename()` still names the file but no longer refuses one, and `FileInfo::sanitizeName()` is a seam `setExtension()` re-fits behind. `FileTest` reads those rows out of the document and reflects over what they name, the way the deny-list test reads the README: hand-copied, the list and the table disagreed on two rows before this sentence was written. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 3 +- CLAUDE.md | 6 +- UPGRADE.md | 36 ++++-- docs/api-reference.md | 12 +- docs/extending.md | 4 + src/Upload/File.php | 194 +++++++++++++++++++++++++++-- tests/Upload/FileTest.php | 171 +++++++++++++++++++++++++ tests/Upload/PropertyProbeFile.php | 39 ++++++ tests/bootstrap.php | 1 + 9 files changed, 440 insertions(+), 26 deletions(-) create mode 100644 tests/Upload/PropertyProbeFile.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a8edeb..d90ac90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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`** diff --git a/CLAUDE.md b/CLAUDE.md index ddb1db1..915dea7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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` 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. diff --git a/UPGRADE.md b/UPGRADE.md index 27fb467..6c46674 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -206,12 +206,31 @@ characters, resolves to a Windows device such as `CON.txt`, or points at a symli destination. A name can no longer select a subdirectory; construct the `FileSystem` with it instead. The shipped `FileInfo` already rewrites or blanks all of these. -**If you subclass `File`.** `$errors` and `$constructorErrors` are gone, replaced by a -`private $errorDetails` holding each error as its parts. Record through `recordError()`, +**If you subclass `File`.** `isValid()`, `upload()` and `uploadValid()` are `final`, so an +override of any of them is a fatal error when your class is loaded. `$errors` and +`$constructorErrors` are gone, replaced by a `private $errorDetails` holding each error as its +parts. Reading, writing or `isset()`-ing either name throws `\LogicException`, and a subclass +that *declares* one of them — or `$errorCodeMessages` — is refused when it is constructed, since +a declared property is in scope and records into itself where nothing reads it. Record through `recordError()`, still `protected`, which now takes the message and the filename separately: `recordError(string $messageId, array $args = [], string $errorCode = ErrorCode::NONE, ?string $filename = null)`. A one-argument call is unchanged. Read with `getErrors()`. -`$errorCodeMessages` is now the method `getUploadErrorMessages()`; override that instead. + +**What is still a seam, and what is not.** + +| If you overrode… | Now do… | +|---|---| +| `File::isValid()`, `File::upload()`, `File::uploadValid()` | Implement `ValidationInterface`, which all three run. It is handed one file at a time, so a check across the whole batch runs before you call `upload()` | +| `$this->errors[]`, `$this->constructorErrors` | `recordError()` to write, `getErrors()`/`getErrorDetails()` to read | +| `File::$errorCodeMessages` | Override `getUploadErrorMessages()` | +| `FileInfo::isUploadedFile()` | Still a seam — pair it with `FileSystem::acceptFilesNotUploadedByPhp()` | +| `FileInfo::getReservedWindowsNames()` | Still a seam | +| `FileInfo::sanitizeName()` | Still a seam for `setName()`, but `setExtension()` re-fits the name afterwards through private code an override does not see | +| `FileSystem::resolveFilename()` | Still a seam, but no longer controls refusals | +| `FileSystem::moveUploadedFile()` | Still there, still `protected` | + +`Validation\Size::scale()` is new, and the seam for the one question the wording leaves open: +it names the unit and formats the number, decimal separator included. **Other changes:** @@ -284,12 +303,11 @@ you would rather branch on a code than read prose. Each of these is listed in full in the [changelog](CHANGELOG.md). -* **`upload()` no longer runs an `isValid()` override.** Both entry points validate - through a private method instead. Validation still runs on every `upload()` call, and - `isValid()` is unchanged when you call it yourself. But a `File` subclass that overrode - `isValid()` to add a check of its own (a quota, a per-tenant policy, an extra scan) no - longer has that check run by `upload()`. **Move it into a `ValidationInterface`**, which - both entry points honour. +* **`upload()` no longer runs an `isValid()` override.** Both entry points validate through a + private method instead, and all three are `final`, so a subclass carrying one is a fatal error + when it loads rather than a check that silently stopped running. Validation still runs on every + `upload()` call, and `isValid()` is unchanged when you call it yourself. **Move a check of your + own into a `ValidationInterface`**, which all three run. * **A `Storage\FileSystem` subclass that overrides `resolveFilename()` no longer decides which names are refused.** `upload()` applies every refusal to whatever the seam returns: `''`, `.`, `..`, a leading dot, control characters and bidi controls, on top of the device diff --git a/docs/api-reference.md b/docs/api-reference.md index 69fa782..77a48a4 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -15,11 +15,11 @@ php.ini, and `InvalidArgumentException` when the key is not in `$_FILES`. | `addValidation(ValidationInterface $validation): File` | Add a single validation rule. Chainable, as are all setters below. | | `addValidations(array $validations): File` | Add several validation rules at once. | | `getValidations(): ValidationInterface[]` | The rules added so far. | -| `isValid(): bool` | Runs `is_uploaded_file()` plus every validation against every file, accumulating failures. Each call resets the error list and re-validates, so it is idempotent. | +| `final isValid(): bool` | Runs `is_uploaded_file()` plus every validation against every file, accumulating failures. Each call resets the error list and re-validates, so it is idempotent. | | `getErrors(): string[]` | All failures from the last validation run (`isValid()`, `upload()` or `uploadValid()`) plus any files that failed to transfer, as `"filename: message"`. A `$_FILES` entry too malformed to name a file is reported without the prefix. Sanitized, but must still be escaped on output. | | `getErrorDetails(): array` | The same failures as their parts: `code` (an [`ErrorCode`](#errorcode) constant, stable across releases), the untranslated `message_id` and its `args`, the sanitized `filename` or `null`, and the finished `message`. For branching on a failure, or rendering it with your own wording. | -| `upload(): bool` | Re-validates, then stores each file via the storage backend. All-or-nothing: one file failing validation stores none of them; call `uploadValid()` in its place to store the ones that passed. Throws `LogicException` when no validations are configured, and `Exception` when validation fails (details in `getErrors()`), when the collection is empty, or when storage fails (details in the exception message). | -| `uploadValid(): bool` | Re-validates, then stores only the files that passed, leaving the rest in `getErrors()`. Returns `true` when every file was stored and `false` when at least one was rejected, counting a file that failed to transfer. Nothing throws for a rejected file, so cleaning up what was already stored is yours on the `false` branch. Throws the same `LogicException` with no validations configured, the same `Exception` on an empty collection, and whatever storage throws. | +| `final upload(): bool` | Re-validates, then stores each file via the storage backend. All-or-nothing: one file failing validation stores none of them; call `uploadValid()` in its place to store the ones that passed. Throws `LogicException` when no validations are configured, and `Exception` when validation fails (details in `getErrors()`), when the collection is empty, or when storage fails (details in the exception message). | +| `final uploadValid(): bool` | Re-validates, then stores only the files that passed, leaving the rest in `getErrors()`. Returns `true` when every file was stored and `false` when at least one was rejected, counting a file that failed to transfer. Nothing throws for a rejected file, so cleaning up what was already stored is yours on the `false` branch. Throws the same `LogicException` with no validations configured, the same `Exception` on an empty collection, and whatever storage throws. | | `getUploadedLocators(): string[]` | Locators returned by the most recent `upload()` or `uploadValid()`, in whatever form the storage backend defines. Multi-file uploads are not atomic, so after a failure this is what needs rolling back. Keyed by collection offset, so the array is sparse after `uploadValid()` and the locator at `$i` still belongs to `$file[$i]`. | | `allowUnvalidatedUploads(): File` | Let `upload()` and `uploadValid()` proceed with no validations configured. What that leaves standing is under [Turning the defaults off](turning-the-defaults-off.md). | | `allowsUnvalidatedUploads(): bool` | Whether that was allowed. An empty `getValidations()` does not say whether that was a decision. | @@ -30,6 +30,12 @@ php.ini, and `InvalidArgumentException` when the key is not in `$_FILES`. | `File::humanReadableToBytes(string $input): int` | Static helper that converts `'5M'` to `5242880`. Accepts B/K/M/G with an optional trailing `B`, and fractions like `'0.5M'`. Throws `InvalidArgumentException` on unparseable input. | | `File::formatUploadFailure(string $clientFilename, int $errorCode): string` | Static: the `getErrors()` string for a file that never arrived, from a client-supplied name and an `UPLOAD_ERR_*` code. Sanitizes the name as the `$_FILES` path does. For reporting a failed transfer outside any collection — a [`FileList`](#filelist)'s `$failures` takes the pairs and words them itself, so it does not need this. A code with no message of its own reads as `Unknown error`. | +The three `final` methods share one sequence: reset the error list, take the re-entrancy lock, +and derive the files that passed from what each validation recorded. Add a check of your own +through [`ValidationInterface`](extending.md). A `File` subclass records failures with the +`protected recordError()`; the list itself is `private`, and reaching for `$errors`, +`$constructorErrors`, `$errorDetails` or `$constructorErrorDetails` throws `LogicException`. + `File` also implements `Countable`, `ArrayAccess` and `IteratorAggregate` over its `FileInfoInterface` objects, and forwards any other method call to them: with one file the call returns that file's value, with several it returns an array of values, and with none it diff --git a/docs/extending.md b/docs/extending.md index 83cffd9..f6fcfdc 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -4,6 +4,10 @@ Two interfaces are the seams: `ValidationInterface` decides whether a file is ac `StorageInterface` decides where it lands. Neither needs a subclass of anything this library ships. +Overriding `File::isValid()` is not a third seam: it, `upload()` and `uploadValid()` are +`final`, since `upload()` validates through a private method that an override never reaches. +A check of your own goes in a `ValidationInterface`, which all three run. + ## Custom validation rules Implement `ValidationInterface` and throw `GravityPdf\Upload\Exception` to reject a file. diff --git a/src/Upload/File.php b/src/Upload/File.php index 63213e4..38c22f1 100644 --- a/src/Upload/File.php +++ b/src/Upload/File.php @@ -40,6 +40,7 @@ use InvalidArgumentException; use LogicException; use IteratorAggregate; +use ReflectionClass; use RuntimeException; /** @@ -72,6 +73,19 @@ */ class File implements ArrayAccess, IteratorAggregate, Countable { + /** What replaced the four error lists, named in the exception each of them now raises */ + private const ERROR_LIST_REPLACEMENT = + 'recordError() to record a failure, and getErrors() or getErrorDetails() to read'; + + /** The members 4.0.0 removed or made `private`, against what a subclass should use instead */ + private const REPLACED_MEMBERS = [ + 'errors' => self::ERROR_LIST_REPLACEMENT, + 'constructorErrors' => self::ERROR_LIST_REPLACEMENT, + 'errorDetails' => self::ERROR_LIST_REPLACEMENT, + 'constructorErrorDetails' => self::ERROR_LIST_REPLACEMENT, + 'errorCodeMessages' => 'the getUploadErrorMessages() method', + ]; + /** The four lifecycle hooks `applyCallback()` will fire, as the property names holding them */ private const LIFECYCLE_CALLBACKS = [ 'beforeValidationCallback', @@ -236,16 +250,18 @@ public function __construct(string $key, StorageInterface $storage) * control-character filter. **Anything every constructor has to do belongs here.** * * It owns the two members neither constructor should set for itself. `$objects` and - * `$errors` are still filled in by each constructor, since they are what reading the + * `$errorDetails` are still filled in by each constructor, since they are what reading the * input produces and the two read entirely different inputs. * * `protected` only so `FileList::__construct()` can call it — a `private` method of this * class is out of reach from a subclass's own constructor. It is not an extension seam: - * overriding it without snapshotting `$errors` costs `isValid()` its idempotence, since + * overriding it without snapshotting the errors costs `isValid()` its idempotence, since * that is the list it resets to. */ protected function init(StorageInterface $storage): void { + $this->guardAgainstReplacedMembers(); + $this->constructorErrorDetails = $this->errorDetails; $this->storage = $storage; } @@ -583,6 +599,151 @@ protected function recordUploadFailure(string $clientFilename, int $errorCode): ); } + /** + * Answer a read of a property this class does not expose + * + * `$this->errors[] = $message` was how a 3.x subclass recorded a failure. An append is a + * read, not a write, so it arrives here rather than at `__set()`. + * + * @return mixed + * @throws LogicException If the property is one this class declares or has replaced + */ + public function __get(string $name) + { + $this->guardPropertyAccess($name); + + /* PHP's own answer for a property that was never declared. It raises this as a notice + before PHP 8.0 and as a warning after; one severity is reported across the range. */ + trigger_error('Undefined property: ' . static::class . '::$' . $name, E_USER_WARNING); + + return null; + } + + /** + * Take a write to a property this class does not expose + * + * A name this class knows nothing about is created, as it was before these methods existed. + * An **append** to such a name is not: PHP calls `__get()` for that and discards the write + * with `Indirect modification of overloaded property`, so a subclass with array state of + * its own has to declare the property rather than let a first append create it. + * + * @param mixed $value + * @throws LogicException If the property is one this class declares or has replaced + */ + public function __set(string $name, $value): void + { + $this->guardPropertyAccess($name); + + $this->$name = $value; + } + + /** + * Answer `isset()`/`empty()` for a property this class does not expose + * + * `if (empty($this->errors))` was the 3.x way to ask whether anything had failed. Without + * this it answers `true` on a collection that rejected every file. + * + * @throws LogicException If the property is one this class declares or has replaced + */ + public function __isset(string $name): bool + { + $this->guardPropertyAccess($name); + + return false; + } + + /** + * Refuse an `unset()` of a property this class does not expose + * + * @throws LogicException If the property is one this class declares or has replaced + */ + public function __unset(string $name): void + { + $this->guardPropertyAccess($name); + } + + /** + * Refuse a subclass the members this class keeps to itself + * + * `$errors` and `$constructorErrors` were `protected` until 4.0.0. A subclass still using + * either name touches a dynamic property nothing reads, so the failure it recorded never + * reaches `getErrors()` — and before PHP 8.2 not even a deprecation is raised. + * `$errorDetails` and `$constructorErrorDetails` replaced them and are `private`, which is + * why the names it might have found in the source are refused as well. + * + * The `property_exists()` arm covers the rest of what this class declares `private`, + * `$running` among them: these methods run in `File`'s scope, so without it a subclass + * assigning to the re-entrancy lock by name would write the real one rather than a dynamic + * property of its own. + * + * @throws LogicException If the property is one this class declares or has replaced + */ + private function guardPropertyAccess(string $name): void + { + if (isset(self::REPLACED_MEMBERS[$name])) { + throw new LogicException(sprintf( + 'File::$%s is not accessible to a subclass. Use %s.', + $name, + self::REPLACED_MEMBERS[$name] + )); + } + + if (property_exists($this, $name)) { + throw new LogicException(sprintf('File::$%s is not accessible here.', $name)); + } + } + + /** + * Refuse a subclass that declares one of the members 4.0.0 replaced + * + * A declared property is in scope, so it never reaches `__get()`/`__set()`: a subclass + * carrying `protected $errors` records every failure into its own array and `getErrors()` + * answers with none, which is the 3.x silent bypass this class otherwise closes. A static + * never dispatches to a magic method at all, which is what leaves `$errorCodeMessages` + * here and nowhere else. + * + * At construction rather than at the touch, so the report does not depend on the subclass + * reaching that line. The answer is per class rather than per instance, and `File` itself + * declares two of these names, so neither walk runs for it. + * + * @throws LogicException If a subclass declares one of them + */ + private function guardAgainstReplacedMembers(): void + { + /** @var array $cleared Classes already walked */ + static $cleared = []; + + $class = static::class; + + if ($class === self::class || isset($cleared[$class]) === true) { + return; + } + + $reflection = new ReflectionClass($class); + + while ($reflection !== false && $reflection->getName() !== self::class) { + foreach (self::REPLACED_MEMBERS as $name => $replacement) { + /* Declared by this class rather than inherited: `File`'s own two are `private`, + which reflection does not report on a subclass at all */ + if ( + $reflection->hasProperty($name) === true + && $reflection->getProperty($name)->getDeclaringClass()->getName() === $reflection->getName() + ) { + throw new LogicException(sprintf( + '%s declares $%s, a name File replaced in 4.0.0 — nothing reads it. Use %s.', + $reflection->getName(), + $name, + $replacement + )); + } + } + + $reflection = $reflection->getParentClass(); + } + + $cleared[$class] = true; + } + /** * Proxy an unknown method to the collection * @@ -628,11 +789,13 @@ public function __call(string $name, array $arguments) * storage failure throws part-way through either, with `getUploadedLocators()` listing * what was already committed. * + * `final` for the reason `isValid()` gives. + * * @return bool * @throws Exception If validation fails, or if there is nothing to upload * @throws LogicException If nothing has been configured to validate against */ - public function upload(): bool + final public function upload(): bool { $this->guardNotReentrant(); $this->running = true; @@ -672,12 +835,14 @@ public function upload(): bool * was: a caller that abandons the request on `false` owns whatever is already on disk, * and `getUploadedLocators()` is the list to undo. * + * `final` for the reason `isValid()` gives. + * * @return bool True when every file was stored, false when at least one was rejected — a * file that never transferred counts as rejected * @throws Exception If there is nothing to upload, or if storage fails * @throws LogicException If nothing has been configured to validate against */ - public function uploadValid(): bool + final public function uploadValid(): bool { $this->guardNotReentrant(); $this->running = true; @@ -726,10 +891,10 @@ private function prepareUpload(): array /** * Refuse a call that re-enters this object from one of its own lifecycle callbacks * - * A run owns `$errors` and `$uploadedFiles`: it resets both at the start, and decides from - * `$errors` which files to hand to storage. A nested call resets them underneath the run - * in progress, so a file that failed can end up in the set that is stored and the locator - * list can lose what was already written. The lock spans the storing as well as the + * A run owns `$errorDetails` and `$uploadedFiles`: it resets both at the start, and decides + * from `$errorDetails` which files to hand to storage. A nested call resets them underneath + * the run in progress, so a file that failed can end up in the set that is stored and the + * locator list can lose what was already written. The lock spans the storing as well as the * validating, since `afterUpload` fires after the validations have finished. * * A callback is given the `FileInfoInterface` it needs; calling back into the collection @@ -810,10 +975,19 @@ public function getUploadedLocators(): array * Re-runs every validation on every call rather than memoizing the result: `upload()` * calls this again, and a `setExtension()` in between must not skip revalidation. * + * `final`, as `upload()` and `uploadValid()` are. The three share one sequence — reset the + * error list to the constructor's, take the re-entrancy lock, derive the files that passed + * from what each iteration recorded — which a partial override breaks without saying so. + * Until 4.0.0 an override of this method also ran on every `upload()`; nothing calls it + * now. A check of your own belongs in a `ValidationInterface`, which all three run, and + * work either side of the storing in the `beforeUpload`/`afterUpload` callbacks. Both + * are handed one file at a time, so a check across the whole batch runs outside the + * collection. + * * @throws LogicException Unabsorbed from a validator: broken code, not a failed file * @throws \Throwable From a user callback */ - public function isValid(): bool + final public function isValid(): bool { $this->guardNotReentrant(); $this->running = true; @@ -852,7 +1026,7 @@ private function runValidations(): array tracked, so an error site added here cannot forget to mark the file failed and hand a rejected upload to storage. Taken before the first hook rather than after, so that guarantee covers the whole iteration and not just the part below - it: `$errors` is `protected`, so a subclass can record from either hook. */ + it: `recordError()` is `protected`, so a subclass can record from either hook. */ $errorCount = count($this->errorDetails); $this->applyCallback('beforeValidationCallback', $fileInfo); diff --git a/tests/Upload/FileTest.php b/tests/Upload/FileTest.php index 399d934..fff7974 100644 --- a/tests/Upload/FileTest.php +++ b/tests/Upload/FileTest.php @@ -1950,6 +1950,177 @@ public function testUploadThrowsWithTheNoFilesCode(): void } } + /** + * A subclass that overrode one of these was calling code `upload()` no longer routes + * through, and a check it added stopped running with nothing said. `final` makes that a + * fatal error when the subclass is loaded. + */ + public function testTheEntryPointsCannotBeOverridden(): void + { + foreach (['isValid', 'upload', 'uploadValid'] as $method) { + $this->assertTrue( + (new \ReflectionMethod(File::class, $method))->isFinal(), + $method . '() must stay final: an override of it does not run' + ); + } + } + + /** + * `$this->errors[] = $message` was the 3.x way to record a failure. The property is gone, + * so the append writes somewhere nothing reads: `getErrors()` never shows the failure, and + * before PHP 8.2 the write is silent. An append is a read, and `empty($this->errors)` is + * an `isset()`, which is why all four routes are covered: left to PHP that last one answers + * `true` on a collection that rejected every file. + * + * @dataProvider provideErrorListAccess + */ + public function testAnErrorListIsUnreachableFromASubclass(string $property, string $access): void + { + $file = new PropertyProbeFile('single', $this->storage); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('recordError()'); + + $file->$access($property, 'rejected'); + } + + /** + * The two names 3.x published and the two `private` ones that replaced them, by each of the + * four routes to them: a subclass that found the new names by reading the source is in the + * same position as one still using the old. + * + * @return array> + */ + public function provideErrorListAccess(): array + { + $cases = []; + + foreach (['errors', 'constructorErrors', 'errorDetails', 'constructorErrorDetails'] as $property) { + foreach (['append', 'assign', 'read', 'isEmpty'] as $access) { + $cases[$property . ': ' . $access] = [$property, $access]; + } + } + + return $cases; + } + + /** + * `__set()` runs in `File`'s scope, so without a guard covering everything the class keeps + * to itself, a subclass assigning to `$running` by name would take the re-entrancy lock + * rather than create a dynamic property of its own. + */ + public function testThePrivateLockIsNotReachableByName(): void + { + $file = new PropertyProbeFile('single', $this->storage); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('not accessible here'); + + $file->assign('running', true); + } + + /** + * `UPGRADE.md`'s seam table tells a 3.x subclass which overrides still hold. A row promising + * a method survived is a promise it is neither `final` nor `private`, and a row in the left + * column is a promise that the member it names really is gone. + * + * The table is read at run time rather than copied, as + * `FileSystemTest::testReadmeDocumentsTheDefaultDenyList()` reads the README's deny-list: + * copied, the two drifted apart in the commit that introduced them. + */ + public function testTheSeamsTheUpgradeGuideNamesAreStillOverridable(): void + { + /* Git checks the document out with CRLF on Windows, where the blank line the table + starts after is "\r\n\r\n" and the pattern below would find nothing */ + $upgrade = str_replace("\r\n", "\n", (string)file_get_contents(dirname(__DIR__, 2) . '/UPGRADE.md')); + + if (preg_match('/What is still a seam, and what is not[^\n]*\n\n((?:\|.*\n)+)/', $upgrade, $table) !== 1) { + $this->fail('UPGRADE.md no longer has a "What is still a seam, and what is not" table'); + } + + /* The rows promising the method is still there, as opposed to those naming a replacement */ + preg_match_all('/^\| `([A-Za-z]+)::([A-Za-z]+)\(\)` \| Still /m', $table[1], $rows, PREG_SET_ORDER); + + $this->assertNotEmpty($rows, 'The seam table no longer promises that anything is still a seam'); + + $classes = [ + 'File' => File::class, + 'FileInfo' => FileInfo::class, + 'FileSystem' => FileSystem::class, + ]; + + foreach ($rows as [, $class, $method]) { + $this->assertArrayHasKey($class, $classes, $class . ' is named in the seam table but not known here'); + + $reflection = new \ReflectionMethod($classes[$class], $method); + + $this->assertFalse($reflection->isFinal(), $class . '::' . $method . '() must stay overridable'); + $this->assertFalse($reflection->isPrivate(), $class . '::' . $method . '() must stay reachable'); + } + + foreach (['errors', 'constructorErrors', 'errorCodeMessages'] as $member) { + $this->assertFalse( + property_exists(File::class, $member), + 'File::$' . $member . ' is documented as gone' + ); + } + } + + /** + * A declared property is in scope, so it never reaches the magic accessors: a subclass + * carrying its own `$errors` records into that and `getErrors()` answers with none, which + * is the 3.x bypass in full. A static never dispatches to a magic method at all, which is + * what leaves `$errorCodeMessages` to the construction-time check alone. + */ + public function testASubclassDeclaringAReplacedMemberIsRefused(): void + { + $subclasses = [ + 'errors' => function (): File { + return new class ('single', $this->storage) extends File { + /** @var string[] */ + protected $errors = []; + }; + }, + 'errorDetails' => function (): File { + return new class ('single', $this->storage) extends File { + /** @var mixed[] */ + protected $errorDetails = []; + }; + }, + 'errorCodeMessages' => function (): File { + return new class ('single', $this->storage) extends File { + /** @var string[] */ + protected static $errorCodeMessages = []; + }; + }, + ]; + + foreach ($subclasses as $member => $construct) { + try { + $construct(); + $this->fail('a subclass declaring $' . $member . ' should have been refused'); + } catch (\LogicException $e) { + $this->assertStringContainsString('$' . $member, $e->getMessage()); + } + } + } + + /** + * Any other property name is left where it was: a subclass with state of its own is not + * what the guard is for. + * + * Silenced because the write creates a dynamic property, which PHP 8.2 deprecates with or + * without this guard and `phpunit.xml` turns into a failure. + */ + public function testAnotherPropertyIsUntouched(): void + { + $file = new PropertyProbeFile('single', $this->storage); + + @$file->assign('tenantId', 7); + + $this->assertSame(7, $file->read('tenantId')); + } + /** @return array> */ public function provideClassesThatRecordErrors(): array { diff --git a/tests/Upload/PropertyProbeFile.php b/tests/Upload/PropertyProbeFile.php new file mode 100644 index 0000000..66484de --- /dev/null +++ b/tests/Upload/PropertyProbeFile.php @@ -0,0 +1,39 @@ +errors` would be the same access under four copies of these methods. + * + * A test picks one by name, so each takes the property first and ignores what it does not use. + */ +class PropertyProbeFile extends File +{ + /** @param mixed $value */ + public function assign(string $property, $value): void + { + $this->$property = $value; + } + + /** @param mixed $value */ + public function append(string $property, $value): void + { + $this->{$property}[] = $value; + } + + /** @return mixed */ + public function read(string $property) + { + return $this->$property; + } + + public function isEmpty(string $property): bool + { + return empty($this->$property); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 091f926..45477ca 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -7,5 +7,6 @@ /* Test doubles that are not themselves test cases, so nothing autoloads them */ require __DIR__ . '/Upload/Storage/ExposedFileSystem.php'; require __DIR__ . '/Upload/VouchedFileInfo.php'; +require __DIR__ . '/Upload/PropertyProbeFile.php'; require __DIR__ . '/Upload/Validation/GermanSize.php'; require __DIR__ . '/Upload/UnicodeSpaces.php';