diff --git a/CHANGELOG.md b/CHANGELOG.md index 0399370..adb190c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -Nothing yet. +### Changed + +- `Singleton` returns to the ecosystem-standard storage shape: + `protected static $instance`, stored by `get_instance()` once the constructor + returns. The class-string-keyed private map introduced for 1.0.0 broke a real + consumer contract — a heavy constructor assigning `static::$instance = $this;` + first so work done during construction can re-enter `get_instance()`. That + early-assignment guard is now the documented, tested pattern. Trade-off, also + documented on the trait: a class using the trait and its subclasses share one + storage slot, so do not call `get_instance()` on a subclass of a singleton. ## [1.0.0] - 2026-07-28 diff --git a/inc/Contracts/Traits/Singleton.php b/inc/Contracts/Traits/Singleton.php index 9f05768..6ebea9d 100644 --- a/inc/Contracts/Traits/Singleton.php +++ b/inc/Contracts/Traits/Singleton.php @@ -18,16 +18,22 @@ */ trait Singleton { /** - * Instances of the classes using this trait, keyed by concrete class name. + * The single instance of the class using this trait. * - * A trait's `static` property is a single storage slot shared by a class and - * its subclasses, so a plain `static $instance` lets a parent and a child that - * both use this trait collide (the child would receive the parent's instance). - * Keying by class name gives each concrete class its own instance. + * 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. * - * @var array + * Limitation: a trait `static` property is one storage slot shared by a class + * and its subclasses. Do not extend a class that uses this trait and call + * `get_instance()` on the child — whichever side is resolved first occupies + * the shared slot for both. + * + * @var ?static */ - private static array $instances = []; + protected static $instance; /** * The single constructor. @@ -40,15 +46,19 @@ protected function __construct() { /** * Get the instance of the class. + * + * 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. */ public static function get_instance(): static { - $class = static::class; - - if ( ! isset( self::$instances[ $class ] ) ) { - self::$instances[ $class ] = new static(); + if ( ! isset( static::$instance ) ) { + static::$instance = new static(); } - return self::$instances[ $class ]; + return static::$instance; } /** diff --git a/tests/Fixtures/FlakySingleton.php b/tests/Fixtures/FlakySingleton.php new file mode 100644 index 0000000..2c79629 --- /dev/null +++ b/tests/Fixtures/FlakySingleton.php @@ -0,0 +1,33 @@ + 3 ) { + // A regression would recurse forever; fail loudly instead. + throw new \RuntimeException( 'Singleton re-entered its own construction.' ); + } + + $this->ready = 'yes'; + + // What a Loader-built class does from inside Main::__construct(). + self::$seen_during_construction = self::get_instance(); + } +} diff --git a/tests/Fixtures/SingletonChild.php b/tests/Fixtures/SingletonChild.php deleted file mode 100644 index 281646c..0000000 --- a/tests/Fixtures/SingletonChild.php +++ /dev/null @@ -1,20 +0,0 @@ - - */ - public static array $construct_counts = []; - - protected function __construct() { - $class = static::class; - - self::$construct_counts[ $class ] = ( self::$construct_counts[ $class ] ?? 0 ) + 1; - } -} diff --git a/tests/SingletonTest.php b/tests/SingletonTest.php index 51d0908..653b08a 100644 --- a/tests/SingletonTest.php +++ b/tests/SingletonTest.php @@ -10,9 +10,9 @@ namespace rtCamp\WPFramework\Tests; use rtCamp\WPFramework\Tests\Fixtures\BareSingleton; -use rtCamp\WPFramework\Tests\Fixtures\SingletonChild; +use rtCamp\WPFramework\Tests\Fixtures\FlakySingleton; +use rtCamp\WPFramework\Tests\Fixtures\ReentrantSingleton; use rtCamp\WPFramework\Tests\Fixtures\SingletonExample; -use rtCamp\WPFramework\Tests\Fixtures\SingletonParent; use rtCamp\WPFramework\Tests\TestCase; final class SingletonTest extends TestCase { @@ -59,32 +59,38 @@ public function test_default_constructor_is_used_when_not_overridden(): void { $this->assertInstanceOf( BareSingleton::class, BareSingleton::get_instance() ); } - public function test_parent_and_subclass_get_separate_instances(): void { - // Regression: the instance store is keyed by class-string, so a subclass - // that inherits get_instance() must not be handed the parent's object - // (or vice versa, depending on which one is resolved first). - $parent = SingletonParent::get_instance(); - $child = SingletonChild::get_instance(); - - $this->assertNotSame( $parent, $child ); - $this->assertInstanceOf( SingletonParent::class, $parent ); - $this->assertInstanceOf( SingletonChild::class, $child ); - - // 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 { + // Regression: the trait documents that a heavy constructor should assign + // `static::$instance = $this;` first, so work done during construction (a + // Main running a Loader) can call get_instance() re-entrantly and receive + // the same object. This pins that contract — the property must stay + // protected and assignable, and the guard must short-circuit a second + // construction. Changing the storage broke a real consumer once already. + $instance = ReentrantSingleton::get_instance(); + + $this->assertSame( 1, ReentrantSingleton::$construct_count ); + $this->assertSame( $instance, ReentrantSingleton::$seen_during_construction ); + + // The re-entrant caller saw the constructor's progress on this object, + // not a fresh copy with default property values. + $this->assertSame( 'yes', ReentrantSingleton::$seen_during_construction->ready ); } - public function test_parent_and_subclass_each_construct_once(): void { - SingletonParent::get_instance(); - SingletonParent::get_instance(); - SingletonChild::get_instance(); - SingletonChild::get_instance(); + public function test_a_throwing_constructor_is_not_left_published(): void { + FlakySingleton::$fail = true; - $this->assertSame( 1, SingletonParent::$construct_counts[ SingletonParent::class ] ?? 0 ); - $this->assertSame( 1, SingletonParent::$construct_counts[ SingletonChild::class ] ?? 0 ); - } + try { + FlakySingleton::get_instance(); + $this->fail( 'Expected the constructor exception to propagate.' ); + } catch ( \RuntimeException $e ) { + $this->assertSame( 'construction failed', $e->getMessage() ); + } + + // Nothing is stored until the constructor returns, so the failed attempt + // cached nothing: once construction can succeed, get_instance() builds a + // fresh, working instance. + FlakySingleton::$fail = false; - public function test_subclass_instance_is_stable_across_calls(): void { - $this->assertSame( SingletonChild::get_instance(), SingletonChild::get_instance() ); + $this->assertInstanceOf( FlakySingleton::class, FlakySingleton::get_instance() ); } }