[huzaifaalmesbah]

What?

Fixes Framework\Factory::get_instance() so each factory subclass gets its own instance instead of all subclasses sharing one.

Closes #613. Part of #302.

Why?

get_instance() stored the instance in self::$instance on the abstract base class. Whichever factory was called first set that property, and every other factory then got the same object back:

Factory\Message::get_instance();      // Factory\Message
Factory\Notification::get_instance(); // also Factory\Message

A notification query loads both models (messages JOIN queue), so calling make() on the wrong factory would produce the wrong model. Nothing calls get_instance() on the factories yet, but this blocks #307.

How?

• Replaced protected static $instance = null with protected static $instances = array(), keyed by static::class, so each subclass stores and returns its own instance.
• Corrected the get_instance() docblock, which said it returns Model. It returns the factory (static).
• Added tests/phpunit/tests/framework/test-factory.php, which checks that:
  • each factory's get_instance() returns an instance of that factory's class
  • Message, Notification and Subscription never return the same object
  • repeated calls on the same subclass return the same object

Testing Instructions

1. npx wp-env start
2. Run the suite:
   npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications vendor/bin/phpunit
3. Run it as multisite too:
   npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications env WP_MULTISITE=1 vendor/bin/phpunit
4. Run only the new tests:
   npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications vendor/bin/phpunit --filter Test_Framework_Factory

Local results: 65 tests, 0 failures on single site and multisite (the 24 skipped are the existing REST controller placeholder tests). composer lint is clean.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Drafting the fix and unit tests. I chose the approach from the issue, reviewed every change, and verified it locally with PHPUnit (single site and multisite) and PHPCS.

[jonathanbossenger]

@huzaifaalmesbah I wanted to check in to see what you thought of the suggestions. If you feel comfortable to leave them as is, just comment here. Thanks

[huzaifaalmesbah]

Thanks for checking in, @jonathanbossenger! I can't see any review comments or suggestions on this PR on my side. Could they still be in a pending review that wasn't submitted? Or did you mean the suggestion on #627? I've added that one now. Happy to look at any suggestions for this PR once I can see them.

[jonathanbossenger]

Optional: What do you think about using self::$instances here (and on the next two lines) instead of static::$instances? The array key would still be static::class.

The fix works as written. The difference only shows up if a subclass ever declares its own $instances property. With static::, that subclass would then store its instance in its own separate array, not in the shared one on Factory. With self::, all instances always live in one array on the base class, whatever the subclasses do.

if ( ! isset( self::$instances[ static::class ] ) ) {
	self::$instances[ static::class ] = new static();
}

return self::$instances[ static::class ];

[huzaifaalmesbah]

Good point, done in 5a9df29. All instances now live in self::$instances on Factory, still keyed by static::class.

[jonathanbossenger]

@huzaifaalmesbah I am so sorry; they were indeed sitting in draft. I've submitted them now. The PR is approved, and the comments are optional, so let me know if you think they are worth updating or not, and then we'll can get this merged.

[huzaifaalmesbah]

Thanks @jonathanbossenger, no worries! I applied both suggestions in 5a9df29. Tests: 66 tests, 0 failures on single site and multisite. PHPCS is clean. Ready to merge from my side.