fix(singleton)!: restore protected static $instance storage - #82
Merged
Conversation
The class-string-keyed private map from #68 broke a real consumer. A theme or plugin Main whose constructor runs a Loader assigns `static::$instance = $this;` as its first statement, so classes built during that work can call get_instance() re-entrantly and receive the same object. theme-elementary does exactly this (inc/Main.php:47) and fataled on v1.0.0 with 'Access to undeclared static property' across its whole CI matrix. That early assignment is not incidental — it is the only workable re-entrancy guard with store-after-construct semantics, so the property is part of the trait's de-facto contract. Restore it and make the contract explicit: - storage is `protected static $instance` again, byte-equivalent in behaviour to the pre-#68 trait; - the heavy-constructor guard pattern is documented on the property and pinned by a regression test (ReentrantSingleton mirrors the theme's exact shape), so the property cannot be silently narrowed again; - the known limitation this re-accepts is documented too: a class and its subclasses share one slot, so get_instance() must not be called on a subclass of a singleton. Nothing in wp-framework or its consumers subclasses one. Also drops the parent/child fixtures and tests that pinned the keyed behaviour, and keeps a test that a throwing constructor caches nothing (true here because nothing is stored until the constructor returns).
pratik-londhe4
approved these changes
Jul 29, 2026
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Restores the Singleton trait’s original protected static $instance storage to re-support ecosystem consumers that publish static::$instance = $this; early as a re-entrancy guard (fixing the fatal seen in downstream themes/plugins).
Changes:
- Revert
Singletonstorage from a class-keyed private map back toprotected static $instance, and document the re-entrancy guard + subclass limitation. - Replace the old inheritance/keying regression fixtures/tests with re-entrancy and “throwing constructor publishes nothing” regression tests.
- Update
CHANGELOG.mdto describe the behavioral contract and trade-off.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| inc/Contracts/Traits/Singleton.php | Restores protected static $instance storage and documents the re-entrancy guard + inheritance limitation. |
| tests/SingletonTest.php | Replaces inheritance/keying tests with re-entrancy and throwing-constructor regression tests. |
| tests/Fixtures/ReentrantSingleton.php | Adds a fixture that exercises early-publish re-entrancy guard behavior. |
| tests/Fixtures/FlakySingleton.php | Adds a fixture whose constructor can throw to ensure failures don’t cache an instance. |
| CHANGELOG.md | Documents the reverted storage shape, the downstream contract, and the inheritance trade-off. |
Comment on lines
+50
to
+54
| * The instance is stored once the constructor returns. If the constructor | ||
| * does work that can call back into `get_instance()`, assign | ||
| * `static::$instance = $this;` as its first statement (see the property | ||
| * docblock) — otherwise the re-entrant call finds nothing stored yet and | ||
| * constructs a second instance, recursing until the stack blows. |
|
|
||
| // The subclass must resolve to its own concrete type, not the parent's. | ||
| $this->assertNotInstanceOf( SingletonChild::class, $parent ); | ||
| public function test_the_documented_early_assignment_guard_supports_reentrant_construction(): void { |
Comment on lines
+69
to
+72
| $instance = ReentrantSingleton::get_instance(); | ||
|
|
||
| $this->assertSame( 1, ReentrantSingleton::$construct_count ); | ||
| $this->assertSame( $instance, ReentrantSingleton::$seen_during_construction ); |
Comment on lines
+23
to
+27
| * Protected on purpose: it is part of the trait's contract. A singleton whose | ||
| * constructor does real work (a plugin/theme `Main` running a Loader) should | ||
| * assign `static::$instance = $this;` as its first statement, so anything | ||
| * built during that work can call `get_instance()` re-entrantly and receive | ||
| * this same object instead of triggering a second construction. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
theme-elementary is fataling across its entire CI matrix on wp-framework v1.0.0 (rtCamp/theme-elementary#747):
#68 changed the trait's storage from
protected static $instanceto aprivate static array $instanceskeyed by class-string. The theme'sMainconstructor doesstatic::$instance = $this;as its first statement (inc/Main.php:47) — a property that no longer exists.Why that line is not incidental
It is the only workable re-entrancy guard under store-after-construct semantics.
Main::__construct()runs a Loader; classes built by the Loader callMain::get_instance()whileMainis still constructing (e.g. a settings page resolving a shared service). With nothing stored yet, that re-entrant call would construct a secondMainand recurse. Reproduced against the trait in isolation:So
protected static $instancewas part of the trait's de-facto contract, and #68 narrowed it based on a repo-local search — the consumer was in a different repo.Change
Restore the previous storage — behaviourally byte-equivalent to the pre-#68 trait (verified by diffing both files with comments stripped):
And make the contract explicit instead of implicit:
static::$instance = $this;first — and pinned by a regression test:ReentrantSingletonmirrors the theme's exact shape (guard, then loader work that re-enters). The property cannot be silently narrowed again without that test failing.staticproperty is one slot shared by a class and its subclasses, soget_instance()must not be called on a subclass of a singleton. Checked every consumer — nothing in wp-framework, theme-elementary, or features-plugin-skeleton subclasses one; the collision fix: wp-framework high-severity audit findings (singleton, encryptor key, feature-flag save) + tests #68 fixed was latent.The parent/child fixtures and tests from #68, which pinned the keyed behaviour, are removed.
Not taken
newInstanceWithoutConstructor, publish, then invoke the constructor) — solves re-entrancy for guard-less consumers and keeps per-class keying, but adds reflection to every firstget_instance()and departs from the singleton shape the WordPress ecosystem writes against. Prototyped and tested, rejected as too clever for what this trait is.AbstractSettingsPage'soption_page_capability_registration toadmin_init— treats one path into the recursion rather than the recursion itself.Semver note
v1.0.0 shipped the keyed storage, so this is technically a change to an inherited member post-1.0. In practice v1.0.0 is hours old, its only two consumers pin
^1.0, and one of them cannot boot on it — this restores the contract they were written against. Needs a v1.0.1 tag on merge; theme-elementary#747 and features-plugin-skeleton#734 then bump their locks.Verified
[OK], phpcs clean.