Skip to content

Close the two routes around the property guard - #45

Merged
jakejackson1 merged 1 commit into
mainfrom
fix/final-property-guard
Aug 25, 2026
Merged

Close the two routes around the property guard#45
jakejackson1 merged 1 commit into
mainfrom
fix/final-property-guard

Conversation

@jakejackson1

Copy link
Copy Markdown
Member

Description

guardPropertyAccess() is private, but the four magic methods that reach it were not final. A subclass overriding one for an ordinary reason of its own — a property bag, a lazy accessor — dropped the guard silently and restored the 3.x bypass:

class BagFile extends FileList {
    private $bag = [];
    public function __set(string $name, $value): void { $this->bag[$name] = $value; }
    public function __get(string $name) { return $this->bag[$name] ?? null; }
    public function __isset(string $name): bool { return isset($this->bag[$name]); }

    public function reject() { $this->errors = ['rejected']; }
    public function anyFailed() { return !empty($this->errors); }
}

Before this PR that class loads, reject() raises no diagnostic at all, getErrors() answers with none, and anyFailed() answers true — which is the collection reporting a failure that nothing can read. guardAgainstReplacedMembers() does not cover it: it catches a declared property, and a magic-property scheme declares nothing.

init() is the same shape. It is protected only so FileList::__construct() can call it, and its docblock already said it is not an extension seam — an override that skips the parent leaves guardAgainstReplacedMembers() unrun and isValid() with no snapshot to reset to, both without a word.

final on all five makes each a fatal error when the subclass loads, for the reason isValid(), upload() and uploadValid() are already final: the check does not stop running quietly.

The cost, which is the part worth arguing about. File is now a class no subclass can give magic properties to at all. I think that is right — guardPropertyAccess() already throws for every name File declares, so any such scheme was full of holes it could not see — but it is a judgement call, not a free win.

No breaking-change entry. Neither init() nor the four magic methods exists in 3.1.0, so there is nothing here to report as a regression. The facts go where 4.0.0 already describes each method: the $errors/$constructorErrors bullet in CHANGELOG.md, and the File::init() signature entry. UPGRADE.md gets a paragraph beside the Size::scale() one rather than a table row, since that table's left column is "if you overrode…" and nobody overrode these.

Testing instructions

testAMethodThatIsNotASeamIsFinal() covers the five, with the reason per method in the assertion message. testTheEntryPointsCannotBeOverridden() is untouched — its fact is distinct and already shipped.

Both holes reproduce on main and are fatal at load here:

$ php -r 'require "vendor/autoload.php"; class X extends GravityPdf\Upload\File { public function __set(string $n, $v): void {} }'
Fatal error: Cannot override final method GravityPdf\Upload\File::__set()

$ php -r 'require "vendor/autoload.php"; class X extends GravityPdf\Upload\FileList { protected function init(GravityPdf\Upload\StorageInterface $s): void {} }'
Fatal error: Cannot override final method GravityPdf\Upload\File::init()

Locally: 724 tests / 1457 assertions with the one expected skip, plus composer lint, composer phpstan and composer check-syntax. composer i18n:pot regenerates byte-identical.

🤖 Generated with Claude Code

`guardPropertyAccess()` is `private`, but the four magic methods that reach it
were not `final`: a subclass overriding one to give itself magic properties
dropped the guard with no diagnostic, and `$this->errors = [...]` went back to
landing where nothing reads it. `init()` was the same shape — `protected` so
`FileList::__construct()` can call it, but an override that skipped the parent
left `guardAgainstReplacedMembers()` unrun and `isValid()` with no snapshot to
reset to.

Neither method existed in 3.1.0, so this is not a break to announce; the facts
go where 4.0.0 already describes them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jakejackson1
jakejackson1 merged commit 44ec6d0 into main Aug 25, 2026
39 checks passed
@jakejackson1
jakejackson1 deleted the fix/final-property-guard branch August 25, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant