Close the two routes around the property guard

CHANGELOG.md

@@ -39,7 +39,7 @@ A security release. New protections are on by default and will refuse some uploa
 * **`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. 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::$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. `__get()`, `__set()`, `__isset()` and `__unset()` are `final`: `guardPropertyAccess()` is `private`, so those four are the only route to it, and a subclass overriding one to give itself magic properties dropped the guard without a diagnostic
 * **`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`**
@@ -85,7 +85,7 @@ A security release. New protections are on by default and will refuse some uploa
 * **`FileList::getSourceKeys(): array`**: the key each file arrived under, by collection offset, so a form field name survives without becoming one. The collection stays keyed `0..n`; writing to an offset or unsetting one drops that key
 * **`File::formatUploadFailure(string $clientFilename, int $errorCode): string`**: static, the `getErrors()` string for an `UPLOAD_ERR_*` failure, sanitized the way the `$_FILES` path sanitizes it, so a `FileList` caller reports failed transfers in the same words. An unrecognized code, `UPLOAD_ERR_OK` included, reads as `Unknown error`
 * **`\GravityPdf\Upload\Filename`** holds the filename rules in one place, so the layers that apply them can't drift apart. Constants `MAX_LENGTH`, `MAX_EXTENSION_LENGTH`, `FALLBACK`, `CONTROL_CHARACTERS`, `BIDI_CONTROLS` and `RESERVED_WINDOWS_NAMES`; `sanitizeNameWithExtension()` for a whole filename, `sanitizeForDisplay()` for prose rather than a name (same character sets; controls collapse to a space, whitespace is trimmed, and none of the length, device-name or `%`/`/` handling applies), plus `acceptExtension()`, `deviceComponent()`, `extensionComponents()`, `normalizeComponents()`, `isReservedDeviceComponent()`, `hasControlCharacters()` and `hasBidiControls()` for reproducing the storage refusals in an implementation of your own. The two character-set constants are bare pattern fragments, so use the predicates rather than passing them to `preg_match()`
-* **`File::init(StorageInterface $storage): void`**: `protected`, the tail both constructors share, so anything added to the `$_FILES` constructor still holds for `FileList`. Not an extension seam: `protected` only because a subclass constructor can't call a parent's `private` method
+* **`File::init(StorageInterface $storage): void`**: `final protected`, the tail both constructors share, so anything added to the `$_FILES` constructor still holds for `FileList`. Not an extension seam: `protected` only because a subclass constructor can't call a parent's `private` method, and `final` because an override skipping it leaves a subclass free to declare `$errors` and costs `isValid()` its idempotence
 * **`FileInfo::resetFactory(): void`** clears a factory installed with `setFactory()`. There was no supported way to undo it in a long-lived process or between tests
 * **Three new `protected` members on `Storage\FileSystem`**: `resolveFilename()` decides the stored name and is the only one meant to be overridden; `reserveDestination()` and `lstatEntry()` exist so a test can reach branches that otherwise need a file-system race. `FileInfo::isReadableFile()` is the guard the metadata accessors share
 * New storage exception messages: `'Invalid destination file name'`, `'Destination is a symbolic link'`, `'Permissions could not be applied to the stored file'` and `'Could not generate a temporary file name'`

UPGRADE.md

@@ -232,6 +232,12 @@ still `protected`, which now takes the message and the filename separately:
 `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.
 
+`File::init()` and the four magic property methods — `__get()`, `__set()`, `__isset()`,
+`__unset()` — are new in 4.0.0 and `final`. `init()` is the tail both constructors share, so
+constructor work of your own goes in your own `__construct()`; the four are the only route to
+the guard that refuses the replaced names above, so declare a property of your own rather than
+making it magic.
+
 **Other changes:**
 
 * `blockExtensions()` called with no argument no longer means the default list: pass

src/Upload/File.php

@@ -254,11 +254,12 @@ public function __construct(string $key, StorageInterface $storage)
      * 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 the errors costs `isValid()` its idempotence, since
-     * that is the list it resets to.
+     * class is out of reach from a subclass's own constructor. `final` because it is not an
+     * extension seam. An override that skips the snapshot costs `isValid()` its idempotence,
+     * since that is the list it resets to; one that skips `guardAgainstReplacedMembers()`
+     * leaves a subclass free to declare `$errors`.
      */
-    protected function init(StorageInterface $storage): void
+    final protected function init(StorageInterface $storage): void
     {
         $this->guardAgainstReplacedMembers();
 
@@ -605,10 +606,15 @@ protected function recordUploadFailure(string $clientFilename, int $errorCode):
      * `$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()`.
      *
+     * `final`, as the other three are. `guardPropertyAccess()` is `private`, so these four are
+     * the only way the guard is reached: a subclass overriding one to give itself magic
+     * properties drops it without a diagnostic, and `$this->errors = [...]` goes back to
+     * landing where nothing reads it.
+     *
      * @return mixed
      * @throws LogicException If the property is one this class declares or has replaced
      */
-    public function __get(string $name)
+    final public function __get(string $name)
     {
         $this->guardPropertyAccess($name);
 
@@ -627,10 +633,12 @@ public function __get(string $name)
      * 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.
      *
+     * `final` for the reason `__get()` gives.
+     *
      * @param mixed $value
      * @throws LogicException If the property is one this class declares or has replaced
      */
-    public function __set(string $name, $value): void
+    final public function __set(string $name, $value): void
     {
         $this->guardPropertyAccess($name);
 
@@ -643,9 +651,11 @@ public function __set(string $name, $value): void
      * `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.
      *
+     * `final` for the reason `__get()` gives.
+     *
      * @throws LogicException If the property is one this class declares or has replaced
      */
-    public function __isset(string $name): bool
+    final public function __isset(string $name): bool
     {
         $this->guardPropertyAccess($name);
 
@@ -655,9 +665,11 @@ public function __isset(string $name): bool
     /**
      * Refuse an `unset()` of a property this class does not expose
      *
+     * `final` for the reason `__get()` gives.
+     *
      * @throws LogicException If the property is one this class declares or has replaced
      */
-    public function __unset(string $name): void
+    final public function __unset(string $name): void
     {
         $this->guardPropertyAccess($name);
     }

tests/Upload/FileTest.php

@@ -1965,6 +1965,43 @@ public function testTheEntryPointsCannotBeOverridden(): void
         }
     }
 
+    /**
+     * The other two `final` groups, both of which fail quietly rather than loudly when a
+     * subclass replaces one: the failure it should have reported never appears in
+     * `getErrors()`, and nothing says so.
+     *
+     * @dataProvider provideMethodsThatAreNotSeams
+     */
+    public function testAMethodThatIsNotASeamIsFinal(string $method, string $reason): void
+    {
+        $this->assertTrue(
+            (new \ReflectionMethod(File::class, $method))->isFinal(),
+            $method . '() must stay final: ' . $reason
+        );
+    }
+
+    /** @return array<string,array{string,string}> */
+    public function provideMethodsThatAreNotSeams(): array
+    {
+        /* `guardPropertyAccess()` is private, so these four are the only route to it. A
+           subclass overriding one to give itself magic properties drops the guard, and
+           `$this->errors = [...]` goes back to landing where nothing reads it. */
+        $guard = 'an override of it removes the guard on the replaced property names';
+
+        /* `protected` so `FileList::__construct()` can call it, not so a subclass can replace
+           it: an override skipping the parent leaves `guardAgainstReplacedMembers()` unrun and
+           `isValid()` with no snapshot to reset to. */
+        $tail = 'an override of it skips the invariant both constructors share';
+
+        return [
+            '__get' => ['__get', $guard],
+            '__set' => ['__set', $guard],
+            '__isset' => ['__isset', $guard],
+            '__unset' => ['__unset', $guard],
+            'init' => ['init', $tail],
+        ];
+    }
+
     /**
      * `$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