Skip to content

fix(singleton)!: restore protected static $instance storage - #82

Merged
aryanjasala merged 1 commit into
mainfrom
fix/singleton-reentrancy
Jul 29, 2026
Merged

fix(singleton)!: restore protected static $instance storage#82
aryanjasala merged 1 commit into
mainfrom
fix/singleton-reentrancy

Conversation

@aryanjasala

Copy link
Copy Markdown
Member

Problem

theme-elementary is fataling across its entire CI matrix on wp-framework v1.0.0 (rtCamp/theme-elementary#747):

Access to undeclared static property rtCamp\Theme\Elementary\Main::$instance
#0 .../wp-framework/inc/Contracts/Traits/Singleton.php(48): Main->__construct()
#1 .../theme-elementary/functions.php(52): Main::get_instance()

#68 changed the trait's storage from protected static $instance to a private static array $instances keyed by class-string. The theme's Main constructor does static::$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 call Main::get_instance() while Main is still constructing (e.g. a settings page resolving a shared service). With nothing stored yet, that re-entrant call would construct a second Main and recurse. Reproduced against the trait in isolation:

without the guard: INFINITE RECURSION (constructed 6 times before the test tripped)
with the guard:    constructs=1, same object, no fatal

So protected static $instance was 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):

protected static $instance;

public static function get_instance(): static {
    if ( ! isset( static::$instance ) ) {
        static::$instance = new static();
    }

    return static::$instance;
}

And make the contract explicit instead of implicit:

  • The guard pattern is documented on the property — a heavy constructor should assign static::$instance = $this; first — and pinned by a regression test: ReentrantSingleton mirrors the theme's exact shape (guard, then loader work that re-enters). The property cannot be silently narrowed again without that test failing.
  • The limitation this re-accepts is documented too: a trait static property is one slot shared by a class and its subclasses, so get_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.
  • A test that a throwing constructor caches nothing (naturally true here: nothing is stored until the constructor returns).

The parent/child fixtures and tests from #68, which pinned the keyed behaviour, are removed.

Not taken

  • Reflection-based publish-before-construct (newInstanceWithoutConstructor, publish, then invoke the constructor) — solves re-entrancy for guard-less consumers and keeps per-class keying, but adds reflection to every first get_instance() and departs from the singleton shape the WordPress ecosystem writes against. Prototyped and tested, rejected as too clever for what this trait is.
  • Deferring AbstractSettingsPage's option_page_capability_ registration to admin_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

  • phpstan [OK], phpcs clean.
  • Trait-level repro: te pattern (guard) → 1 construct, same object; plain consumer → fine; throwing constructor → retry works.
  • The full wp-env matrix runs in this PR's CI.

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).
Copilot AI review requested due to automatic review settings July 29, 2026 11:19
@aryanjasala
aryanjasala merged commit 569f8ee into main Jul 29, 2026
22 of 23 checks passed
@aryanjasala
aryanjasala deleted the fix/singleton-reentrancy branch July 29, 2026 11:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Singleton storage from a class-keyed private map back to protected 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.md to 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.
Comment thread tests/SingletonTest.php

// 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 thread tests/SingletonTest.php
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.
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.

3 participants