diff --git a/CHANGELOG.md b/CHANGELOG.md index e0a9ecd..586040f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [3.3.2] + +### Fixed + +- Route controllers are now built by the application container. Constructor + dependencies follow the same rules as action-method parameters: bindings, + singletons, interfaces bound in the container, and default values for scalar + parameters. Previously `Route` built controllers with its own injector, which + ignored the container. A singleton injected into the constructor was then a + different object from the same singleton injected into the action, a bound + interface failed with `Class "..." does not exist`, a scalar parameter failed + even when it had a default, and unbound services were shared between unrelated + controllers. A controller is now built fresh on each dispatch; bind it with + `app()->singleton()` to share one instance. `Route::make()` keeps its signature + and delegates to the container. The protected static cache it used is removed: + `Route::$instances`, `cache()`, `is_cached()` and `get_cached()`. See + `docs/routing-examples.md`. + ## [3.3.1] ### Added diff --git a/docs/routing-examples.md b/docs/routing-examples.md index 7287547..19a6d41 100644 --- a/docs/routing-examples.md +++ b/docs/routing-examples.md @@ -100,6 +100,37 @@ public function show(EventShowRequest $request, Event $event, EventService $serv **Expected:** `GET /wp-json/kirki/v1/events/42` resolves `{event}` from the route, authorizes/validates via the Form Request, and returns JSON. +### Controller constructor injection + +Controllers are built by the application container, the same way as `app()->make()`. A constructor therefore gets bindings, singletons, interfaces bound in the container, and the default value of any scalar parameter, just as action-method parameters do: + +```php +use function Framework\app; + +// In a service provider +app()->singleton(Cart::class, fn () => new Cart()); +app()->bind(PaymentGateway::class, fn () => new StripeGateway()); + +class CartController +{ + protected $cart; + + public function __construct(Cart $cart, PaymentGateway $gateway, int $limit = 10) + { + $this->cart = $cart; + } + + public function add(CartAddRequest $request, Cart $cart) + { + // $this->cart and $cart are the same singleton + } +} +``` + +A controller is built fresh on every dispatch. To share one instance across dispatches in the same PHP process (for example REST batch requests), bind the controller itself with `app()->singleton(CartController::class)`. + +**Expected:** The constructor and the action method receive the same `Cart` instance, `$gateway` is a `StripeGateway`, and `$limit` is `10`. + --- diff --git a/example/app/Http/Controllers/TestController.php b/example/app/Http/Controllers/TestController.php new file mode 100644 index 0000000..5af2fe3 --- /dev/null +++ b/example/app/Http/Controllers/TestController.php @@ -0,0 +1,29 @@ +service = $service; + $this->service->increase(); + } + + public function get(Request $request, TestService $service) + { + return response()->json([ + 'message' => 'Test Controller Loaded', + 'check' => $this->service->value() === $service->value() + ]); + } +} diff --git a/example/app/Providers/TestServiceProvider.php b/example/app/Providers/TestServiceProvider.php index f3b38c1..fe30bac 100644 --- a/example/app/Providers/TestServiceProvider.php +++ b/example/app/Providers/TestServiceProvider.php @@ -2,6 +2,7 @@ namespace Example\App\Providers; +use Example\App\Services\TestService; use Framework\Http\Request; use Framework\RateLimiting\Limit; use Framework\ServiceProvider; @@ -14,6 +15,7 @@ class TestServiceProvider extends ServiceProvider public function register() { $config = config('app.name'); + $this->app->singleton(TestService::class); } public function boot() diff --git a/example/app/Services/TestService.php b/example/app/Services/TestService.php new file mode 100644 index 0000000..03acaa1 --- /dev/null +++ b/example/app/Services/TestService.php @@ -0,0 +1,23 @@ +value; + } + + public function increase(int $amount = 1) + { + $this->value += $amount; + } + + public function decrease(int $amount = 1) + { + $this->value -= $amount; + } +} diff --git a/example/routes/api.php b/example/routes/api.php index 1da0e72..1f1e36d 100644 --- a/example/routes/api.php +++ b/example/routes/api.php @@ -5,6 +5,7 @@ use Example\App\Http\Controllers\BlogController; use Example\App\Http\Controllers\EventsController; use Example\App\Http\Controllers\SpeakersController; +use Example\App\Http\Controllers\TestController; use Example\App\Http\Requests\ExampleRequest; use Example\App\Models\Blog; use Example\App\Models\Event; @@ -90,3 +91,6 @@ 'cached' => Cache::get('sample'), ]); })->throttle(5, 1); + + +Route::get('/new-route', [TestController::class, 'get']); \ No newline at end of file diff --git a/openspec/changes/archive/2026-11-12-route-controller-container-resolution/.openspec.yaml b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/.openspec.yaml new file mode 100644 index 0000000..55ab199 --- /dev/null +++ b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-12 diff --git a/openspec/changes/archive/2026-11-12-route-controller-container-resolution/design.md b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/design.md new file mode 100644 index 0000000..f6a2224 --- /dev/null +++ b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/design.md @@ -0,0 +1,74 @@ +## Context + +See proposal.md (Why) for the defect. The relevant code paths are: + +- `Route::resolve_controller()` (`src/Route.php:1898`) is the only place that builds a controller. It runs once per dispatch, from `dispatch_with_request()`, for both REST and site routes. The permission callback and the middleware pipeline never build the controller. +- `Route::make()` (`src/Route.php:1198`) is a reflection injector with its own circular-dependency check. It memoizes every instance it builds, controllers and their dependencies alike, in `protected static $instances` through `cache()`, `is_cached()` and `get_cached()`. +- `Route::resolve_abstracts()` (`src/Route.php:1478`) resolves action-method dependencies through `app()->make()`. +- `Container::autowire()` (`src/Container.php`) already does everything `Route::make()` does, plus the parts it gets wrong: bindings, singletons, aliases, scalar defaults, a "not instantiable" check, and circular-dependency detection with the full chain in the message. + +## Goals / Non-Goals + +**Goals:** + +- Make the container the single dependency injector for controller construction. +- Keep `Route::make()` callable and overridable by subclasses with an unchanged signature. + +**Non-Goals:** + +- Changing how `Container` resolves dependencies. In particular, `?SomeInterface $x = null` with an unbound interface still throws instead of receiving `null`. That happened under `Route::make()` too, so this change makes nothing worse. Fixing it changes resolution for every `app()->make()` caller and belongs in its own change. +- Memoizing controllers per `Route` object, which is the Laravel approach. Nothing here needs it; see Decision 1. +- Changing how action-method or closure dependencies are categorized or resolved. + +## Decisions + +### 1. Controllers are built fresh per dispatch, not cached + +`resolve_controller()` gets the controller from the container on every dispatch. A consumer who wants one shared instance binds the controller, or the dependency, with `app()->singleton()`. + +**Why:** the static cache never saves work inside a single request, because the controller is built exactly once per dispatch either way. A process sees more than one dispatch only for REST `batch/v1`, `rest_do_request()`, and block-editor preloads. There the static cache is a hazard, because any per-request state on the controller would leak into the next sub-request. kirki-ecommerce's 28 controllers write properties only in their constructors, so a fresh instance behaves identically for them. The cache also forces `TestCase::reset_route_state()` to clear `Route::$instances` between tests. + +**Alternatives considered:** + +- *Memoize on the `Route` instance (Laravel).* This adds state and a second lifetime rule for no current benefit. +- *Keep the process-wide static cache, filled by the container.* This keeps the cross-controller sharing (symptom 4) and the hidden second lifetime the report complains about. + +### 2. `Route::make()` becomes a one-line call to the container; the cache members are deleted + +```php +protected function make(string $abstract, array $resolving = []) +{ + return app()->make($abstract); +} +``` + +`$resolving` stays in the signature so that a subclass that overrides or calls `make()` keeps a compatible signature. It goes unused, and the docblock says it is kept for compatibility. `resolve_controller()` keeps calling `$this->make($controller)`, so `make()` stays the one method a subclass overrides to change how controllers are built. + +`protected static $instances`, `cache()`, `is_cached()` and `get_cached()` are deleted. With nothing filling the cache they would be dead code, and keeping them invites someone to wire the second cache back in. The `use ReflectionClass;` import becomes unused and is removed. `Exception`, `throw_if` and `throw_unless` are still used elsewhere in `Route.php` and stay. + +**Alternatives considered:** + +- *Delete `make()` too.* Any subclass that calls `$this->make()` would hit a fatal error. The one-line method is cheap compatibility. +- *Keep everything and mark it deprecated.* This leaves the broken injector reachable, and a deprecated cache that nothing fills would be misleading. + +### 3. The existing `class_exists($controller)` guard stays + +`resolve_controller()` already throws `InvalidRoutActionException('Controller X not found')` before calling `make()`. That message is more useful at the routing layer than the container's reflection error, so it stays unchanged. + +### 4. Tests + +- Replace `test_make_resolves_constructor_dependencies_and_caches_instances` with a test that two `make()` calls return two distinct instances. The old test asserts the behaviour being removed. +- Add cases for each spec scenario: a constructor and action method that share a singleton, a bound interface, a closure-built service with a scalar constructor, a scalar default, a singleton-bound controller reused across two dispatches, and an unbound dependency that is not shared across controllers. +- `test_make_detects_circular_dependencies` keeps working unchanged. The container's message starts with `Circular dependency detected:`. +- Remove the `'instances' => []` entry from `TestCase::reset_route_state()`. The `hasProperty()` guard would skip it anyway, but it would be a stale reference. + +## Risks / Trade-offs + +- [A consumer subclass reads `Route::$instances` or calls the cache helpers] → It fails loudly with an undefined property or method, not silently. No known consumer does this. The CHANGELOG entry names the removed members. +- [A consumer controller relied on state persisting across dispatches in one process] → It only matters for batch or internal REST dispatch. The fix is to bind the controller as a singleton, and the CHANGELOG entry says so. kirki-ecommerce controllers are stateless. +- [Unbound services that were accidentally shared between controllers are now separate] → This is the intended fix (symptom 4). Anything that must be shared should be a container singleton. +- [Error messages change for bad constructors: "not instantiable" instead of "is abstract" / "does not exist"] → Only error text changes, and the container's messages are more accurate. + +## Migration Plan + +Ships as 3.3.2 under `### Fixed`. Consumers update the Composer dependency and re-scope. No code change is needed unless a subclass touches the removed cache members. Rollback means reverting to 3.3.1. diff --git a/openspec/changes/archive/2026-11-12-route-controller-container-resolution/proposal.md b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/proposal.md new file mode 100644 index 0000000..5870e12 --- /dev/null +++ b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/proposal.md @@ -0,0 +1,36 @@ +## Why + +`Route::make()` (`src/Route.php:1198`) builds controllers with its own reflection injector and a process-wide static cache. It never asks the application container. Action-method parameters, by contrast, go through `app()->make()`. So a controller's constructor follows different rules from everything else in the framework: + +- A container singleton injected into the constructor is a different object from the same singleton injected into the action method. Nothing errors, so the bug is silent. +- An interface bound in the container fails with `Class "X" does not exist`. +- A scalar parameter fails with `must be a class type, not a built-in type`, even when it has a default value. +- Unbound services are shared between unrelated controllers through `Route::$instances`. + +All four were reproduced against the current code. kirki-ecommerce is one step away from hitting this. `ShippingService` is a container singleton built from `array $shipping_settings`, and `OrderCalculationContextService` injects it. Any controller constructor that reaches it would crash. + +## What Changes + +- Controllers are built by the application container on every dispatch, so constructor dependencies follow the same rules as `app()->make()`: bindings, singletons, bound interfaces, scalar defaults, and circular-dependency detection. +- `Route::make()` keeps its signature and delegates to `app()->make()`. `resolve_controller()` keeps calling `$this->make()`, so subclasses still have one method to override. +- **BREAKING (narrow):** the protected static cache is removed: `Route::$instances`, `cache()`, `is_cached()`, `get_cached()`. Only a subclass of `Route` that touches these members is affected. +- Behaviour change: a controller is no longer reused for the whole process. It is built fresh per dispatch, and so are its unbound dependencies. Consumers who want one shared instance bind it as a singleton. +- CHANGELOG `## [3.3.2]` → `### Fixed` entry; a "Controller constructor injection" subsection in `docs/routing-examples.md`. + +## Capabilities + +### New Capabilities + +- `route-controller-resolution`: how a route builds its controller instance: through the application container, fresh per dispatch, with the same resolution rules as action-method dependencies. + +### Modified Capabilities + +None. No existing spec covers routing. + +## Impact + +- **Code:** `src/Route.php`: `make()`, the static `$instances` property, and the `cache()`, `is_cached()` and `get_cached()` methods. +- **Tests:** `tests/Unit/RouteDiTest.php` (replace the caching test, add container-resolution cases); `tests/Unit/TestCase.php` (drop the `'instances'` reset). +- **Docs:** `docs/routing-examples.md`, `CHANGELOG.md`. +- **Consumers:** kirki-ecommerce controllers need no change. They inject only concrete services and keep no state between dispatches. Controller constructors become safe for container-bound services such as `ShippingService`, `MoneyManager` and `PaymentManager`. +- **Scoping:** none. `app()->make()` takes the class-string it is given, so nothing new is built from string literals. diff --git a/openspec/changes/archive/2026-11-12-route-controller-container-resolution/specs/route-controller-resolution/spec.md b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/specs/route-controller-resolution/spec.md new file mode 100644 index 0000000..28fa7c3 --- /dev/null +++ b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/specs/route-controller-resolution/spec.md @@ -0,0 +1,62 @@ +## Purpose + +Defines how a route builds the controller instance for a controller action, so that controller +constructors receive dependencies under the same rules as action-method parameters and every other +service the application container resolves. + +## ADDED Requirements + +### Requirement: Controllers are resolved through the application container +When a route with a `[Controller::class, 'method']` action is dispatched, on a REST or a site route, +the framework SHALL obtain the controller instance from the application container. The controller's +constructor dependencies SHALL be resolved by the same rules as `app()->make()`. A dependency that +is bound or registered as a singleton SHALL receive the bound instance, and an interface with a +binding SHALL receive the bound implementation. + +#### Scenario: Constructor and action method share a container singleton +- **WHEN** a class is registered as a singleton in the container, and a controller injects that class both in its constructor and in the dispatched action method +- **THEN** the constructor and the action method receive the same instance, and it is the container's singleton + +#### Scenario: Constructor receives a bound interface +- **WHEN** a controller constructor type-hints an interface that has a binding in the container +- **THEN** the controller is built with the bound implementation, and no "class does not exist" error is raised + +#### Scenario: Constructor receives a container-built service +- **WHEN** a controller constructor type-hints a class that is bound with a factory closure, whose own constructor takes non-class arguments supplied by that closure +- **THEN** the controller is built with the instance the closure returns + +### Requirement: Scalar constructor parameters with defaults are accepted +A controller constructor parameter with a built-in type, or with no type, SHALL NOT cause +resolution to fail when it declares a default value. The default value SHALL be passed. A +parameter of that kind with no default SHALL fail with the container's error for an unresolvable +primitive. + +#### Scenario: Scalar parameter with a default +- **WHEN** a controller constructor is `__construct(Logger $logger, int $limit = 10)` +- **THEN** the controller is built with a resolved `Logger` and `$limit` equal to `10` + +### Requirement: A controller is built fresh for each dispatch +The framework SHALL NOT keep its own cache of controller instances or of their constructor +dependencies. Each dispatch SHALL obtain the controller from the container. That SHALL yield a new +instance unless the consumer bound the controller, or the dependency, as a singleton. Two different +controllers SHALL share an unbound dependency only if the container would share it. + +#### Scenario: Two dispatches build two controllers +- **WHEN** the same controller route is dispatched twice in one PHP process, and the controller is not bound as a singleton +- **THEN** each dispatch receives a distinct controller instance + +#### Scenario: Singleton-bound controller is reused +- **WHEN** a controller class is registered as a singleton in the container, and its route is dispatched twice in one PHP process +- **THEN** both dispatches receive the same controller instance + +#### Scenario: Unbound dependency is not shared across controllers +- **WHEN** two different controllers each inject the same unbound concrete service in their constructors +- **THEN** each controller receives its own instance of that service + +### Requirement: Circular constructor dependencies are reported +Resolving a controller whose constructor dependencies form a cycle SHALL fail with an exception +whose message identifies the circular dependency. It SHALL NOT recurse without limit. + +#### Scenario: Circular dependency in a controller's constructor graph +- **WHEN** a controller depends on class A, A depends on B, and B depends on A +- **THEN** resolution throws an exception whose message contains "Circular dependency detected" diff --git a/openspec/changes/archive/2026-11-12-route-controller-container-resolution/tasks.md b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/tasks.md new file mode 100644 index 0000000..074ff71 --- /dev/null +++ b/openspec/changes/archive/2026-11-12-route-controller-container-resolution/tasks.md @@ -0,0 +1,29 @@ +## 1. Failing tests first + +- [x] 1.1 In `tests/Unit/RouteDiTest.php`, replace `test_make_resolves_constructor_dependencies_and_caches_instances` with a test that `make()` resolves constructor dependencies and that two calls return two distinct instances. Verify: it fails against the current static cache. +- [x] 1.2 Add a test where a class registered with `app()->singleton()` is injected into both the controller constructor and the action method of a dispatched controller route. Assert that both see the same instance, and that it is `app()->make()`'s instance. Drive the dispatch through `dispatch_site()`, as `tests/Unit/Routing/SiteRouterTest.php` does, or call `resolve_controller()` with a `Request`. Verify: it fails today. +- [x] 1.3 Add a test where the controller constructor type-hints an interface bound with `app()->bind()`, and assert that it receives the bound implementation. Verify: it fails today with `Class "..." does not exist`. +- [x] 1.4 Add a test where the constructor injects a service bound by a factory closure, and that service's own constructor takes an `array` (like kirki-ecommerce's `ShippingService`). Assert that the controller holds the closure's instance. Verify: it fails today with "must be a class type". +- [x] 1.5 Add a test for `__construct(RouteDiLeafService $leaf, int $limit = 10)` that asserts `$limit === 10`. Verify: it fails today with "must be a class type". +- [x] 1.6 Add a test where a controller bound as a singleton yields the same instance from two `make()` calls. +- [x] 1.7 Add a test where two different controllers that inject the same unbound concrete service receive different service instances. Verify: it fails today, because the static cache shares them. +- [x] 1.8 Keep `test_make_detects_circular_dependencies` unchanged. Confirm it still expects "Circular dependency detected". + +## 2. Route the controller through the container + +- [x] 2.1 In `src/Route.php`, replace the body of `make()` with `return app()->make($abstract);`. Keep the signature and the `$resolving` parameter. Update the docblock to say that it resolves through the application container and that `$resolving` is kept only for signature compatibility. Drop the `@throws \Exception` tag if nothing in the body throws. +- [x] 2.2 Leave `resolve_controller()` calling `$this->make($controller)`, and leave its `class_exists()` guard unchanged. +- [x] 2.3 Delete `protected static $instances`, `cache()`, `is_cached()` and `get_cached()` from `src/Route.php`. +- [x] 2.4 Remove the `use ReflectionClass;` import if nothing else in `Route.php` uses it. Keep `Exception`, `throw_if` and `throw_unless`, which are still used. +- [x] 2.5 Remove `'instances' => []` from `reset_route_state()` in `tests/Unit/TestCase.php`. +- [x] 2.6 Run `composer test:unit -- --filter 'RouteDiTest'` and `composer test:unit -- --filter 'SiteRouterTest'`. Verify: all of section 1 passes. + +## 3. Documentation + +- [x] 3.1 In `docs/routing-examples.md`, add a "Controller constructor injection" subsection beside the existing controller examples, without adding a new numbered section. Cover: constructors receive container services (bindings, singletons, bound interfaces, scalars with defaults); a controller is built fresh per dispatch; bind it with `app()->singleton()` to share one instance. Update the table of contents if subsections are listed there. +- [x] 3.2 Add `## [3.3.2]` → `### Fixed` to the top of `CHANGELOG.md`, following the 3.3.1 entry's style. State that controller constructors now resolve through the container (bindings, singletons, interfaces, scalar defaults), that controllers are built fresh per dispatch (bind as a singleton to share one), and that the protected `Route::$instances`, `cache()`, `is_cached()` and `get_cached()` were removed. + +## 4. Verification + +- [x] 4.1 Run `composer phpcs && composer test:unit`. Verify: both pass with no new warnings. +- [x] 4.2 Re-run `/private/tmp/claude-501/-Users-ahamed-development-ollyo-wordpress-framework/b9a69640-08a3-4fdc-9414-fd9e13cf65e3/scratchpad/repro.php`, or its equivalent. Verify: the Cart instances are the same, `PaymentGateway` resolves, and `$limit` defaults to 10. diff --git a/openspec/specs/route-controller-resolution/spec.md b/openspec/specs/route-controller-resolution/spec.md new file mode 100644 index 0000000..6ca83c7 --- /dev/null +++ b/openspec/specs/route-controller-resolution/spec.md @@ -0,0 +1,62 @@ +## Purpose + +Defines how a route builds the controller instance for a controller action, so that controller +constructors receive dependencies under the same rules as action-method parameters and every other +service the application container resolves. + +## Requirements + +### Requirement: Controllers are resolved through the application container +When a route with a `[Controller::class, 'method']` action is dispatched, on a REST or a site route, +the framework SHALL obtain the controller instance from the application container. The controller's +constructor dependencies SHALL be resolved by the same rules as `app()->make()`. A dependency that +is bound or registered as a singleton SHALL receive the bound instance, and an interface with a +binding SHALL receive the bound implementation. + +#### Scenario: Constructor and action method share a container singleton +- **WHEN** a class is registered as a singleton in the container, and a controller injects that class both in its constructor and in the dispatched action method +- **THEN** the constructor and the action method receive the same instance, and it is the container's singleton + +#### Scenario: Constructor receives a bound interface +- **WHEN** a controller constructor type-hints an interface that has a binding in the container +- **THEN** the controller is built with the bound implementation, and no "class does not exist" error is raised + +#### Scenario: Constructor receives a container-built service +- **WHEN** a controller constructor type-hints a class that is bound with a factory closure, whose own constructor takes non-class arguments supplied by that closure +- **THEN** the controller is built with the instance the closure returns + +### Requirement: Scalar constructor parameters with defaults are accepted +A controller constructor parameter with a built-in type, or with no type, SHALL NOT cause +resolution to fail when it declares a default value. The default value SHALL be passed. A +parameter of that kind with no default SHALL fail with the container's error for an unresolvable +primitive. + +#### Scenario: Scalar parameter with a default +- **WHEN** a controller constructor is `__construct(Logger $logger, int $limit = 10)` +- **THEN** the controller is built with a resolved `Logger` and `$limit` equal to `10` + +### Requirement: A controller is built fresh for each dispatch +The framework SHALL NOT keep its own cache of controller instances or of their constructor +dependencies. Each dispatch SHALL obtain the controller from the container. That SHALL yield a new +instance unless the consumer bound the controller, or the dependency, as a singleton. Two different +controllers SHALL share an unbound dependency only if the container would share it. + +#### Scenario: Two dispatches build two controllers +- **WHEN** the same controller route is dispatched twice in one PHP process, and the controller is not bound as a singleton +- **THEN** each dispatch receives a distinct controller instance + +#### Scenario: Singleton-bound controller is reused +- **WHEN** a controller class is registered as a singleton in the container, and its route is dispatched twice in one PHP process +- **THEN** both dispatches receive the same controller instance + +#### Scenario: Unbound dependency is not shared across controllers +- **WHEN** two different controllers each inject the same unbound concrete service in their constructors +- **THEN** each controller receives its own instance of that service + +### Requirement: Circular constructor dependencies are reported +Resolving a controller whose constructor dependencies form a cycle SHALL fail with an exception +whose message identifies the circular dependency. It SHALL NOT recurse without limit. + +#### Scenario: Circular dependency in a controller's constructor graph +- **WHEN** a controller depends on class A, A depends on B, and B depends on A +- **THEN** resolution throws an exception whose message contains "Circular dependency detected" diff --git a/src/Route.php b/src/Route.php index eab1e19..6ca994b 100644 --- a/src/Route.php +++ b/src/Route.php @@ -30,7 +30,6 @@ use Framework\View\View; use Framework\Wordpress\Constants\HookNames; use InvalidArgumentException; -use ReflectionClass; use ReflectionFunction; use ReflectionMethod; use ReflectionNamedType; @@ -226,15 +225,6 @@ class Route */ protected $param_types = []; - /** - * Array of class instances. - * - * @var array - * - * @since 1.0.0 - */ - protected static $instances = []; - /** * The resolved request. * @@ -1141,127 +1131,21 @@ public function register() } /** - * Cache a class instance. - * - * @param string $abstract The class name to bind - * @param object $instance The instance of the class - * - * @return void - * - * @since 1.0.0 - */ - protected function cache(string $abstract, $instance) - { - static::$instances[$abstract] = $instance; - } - - /** - * Check if a class instance is cached. + * Resolve a class through the application container. * - * @param string $abstract The class name to check + * Controllers are built here, so their constructor dependencies follow the same bindings, + * singletons and defaults as every other service the container resolves. * - * @return bool - * - * @since 1.0.0 - */ - protected function is_cached(string $abstract) - { - return isset(static::$instances[$abstract]); - } - - /** - * Get a cached class instance. - * - * @param string $abstract The class name to get - * - * @return object - * - * @since 1.0.0 - */ - protected function get_cached(string $abstract) - { - return static::$instances[$abstract]; - } - - /** - * Resolve a class and its dependencies. - * - * @param string $abstract The class name to resolve - * @param array $resolving Stack of classes being resolved (for + * @param string $abstract The class name to resolve. + * @param array $resolving Unused; kept so subclasses that call or override this keep a compatible signature. * * @return object The resolved instance * - * @throws \Exception - * * @since 1.0.0 */ protected function make(string $abstract, array $resolving = []) { - if ($this->is_cached($abstract)) { - return $this->get_cached($abstract); - } - - throw_if( - in_array($abstract, $resolving, true), - sprintf('Circular dependency detected for class "%s".', $abstract), - Exception::class - ); - - throw_unless(class_exists($abstract), sprintf('Class "%s" does not exist.', $abstract)); - - $reflector = new ReflectionClass($abstract); - - throw_if( - $reflector->isAbstract(), - sprintf('Class "%s" is abstract and cannot be instantiated.', $abstract), - Exception::class - ); - - $constructor = $reflector->getConstructor(); - - if (!$constructor) { - return new $abstract(); - } - - throw_unless( - $constructor->isPublic(), - sprintf('Class "%s" has a non-public constructor and cannot be instantiated.', $abstract), - Exception::class - ); - - $dependencies = []; - $resolving[] = $abstract; - - foreach ($constructor->getParameters() as $parameter) { - $type = $parameter->getType(); - - throw_unless( - $type !== null, - sprintf( - 'Parameter "%s" is missing a type hint in the constructor. Please add a class type hint.', - $parameter->getName() - ), - Exception::class - ); - - throw_if( - $type->isBuiltin(), - sprintf( - 'Parameter "%s" must be a class type, not a built-in type. Please specify a valid class dependency.', // phpcs:ignore Generic.Files.LineLength.TooLong - $parameter->getName() - ), - Exception::class - ); - - $dependencies[] = $this->is_cached($type->getName()) - ? $this->get_cached($type->getName()) - : $this->make($type->getName(), $resolving); - } - - $instance = $reflector->newInstanceArgs($dependencies); - $this->cache($abstract, $instance); - - return $instance; + return app()->make($abstract); } /** diff --git a/tests/Unit/RouteDiTest.php b/tests/Unit/RouteDiTest.php index c5c586b..5e71f27 100644 --- a/tests/Unit/RouteDiTest.php +++ b/tests/Unit/RouteDiTest.php @@ -9,6 +9,8 @@ use Framework\Route; use WP_REST_Request; +use function Framework\app; + class RouteDiTest extends TestCase { protected function setUp(): void @@ -48,7 +50,7 @@ public function test_where_replaces_route_parameter_pattern(): void $this->assertSame('products/(?P[0-9]+)', $formatted); } - public function test_make_resolves_constructor_dependencies_and_caches_instances(): void + public function test_make_resolves_constructor_dependencies_fresh_on_each_call(): void { $route = Route::get('ping', [RouteDiTestController::class, 'show']); @@ -56,8 +58,78 @@ public function test_make_resolves_constructor_dependencies_and_caches_instances $second = $this->invoke_route_method($route, 'make', RouteDiServiceWithDependency::class); $this->assertInstanceOf(RouteDiServiceWithDependency::class, $first); + $this->assertInstanceOf(RouteDiLeafService::class, $first->leaf); + $this->assertNotSame($first, $second); + $this->assertNotSame($first->leaf, $second->leaf); + } + + public function test_controller_constructor_and_action_share_container_singleton(): void + { + $this->bootstrap_application(); + app()->singleton(RouteDiCart::class, fn () => new RouteDiCart()); + + $route = Route::get('cart', [RouteDiCartController::class, 'add']); + $rest_request = new WP_REST_Request('GET', '/framework/v1/cart'); + $this->invoke_route_method($route, 'resolve_permission_callback', $rest_request); + + $callback = $this->invoke_route_method($route, 'resolve_route'); + $result = $callback($rest_request); + + $this->assertIsArray($result); + $this->assertSame($result['constructor'], $result['action']); + $this->assertSame(app()->make(RouteDiCart::class), $result['constructor']); + } + + public function test_make_resolves_constructor_interface_bound_in_container(): void + { + app()->bind(RouteDiGateway::class, fn () => new RouteDiStripeGateway()); + $route = Route::get('pay', [RouteDiPaymentController::class, 'show']); + + $controller = $this->invoke_route_method($route, 'make', RouteDiPaymentController::class); + + $this->assertInstanceOf(RouteDiStripeGateway::class, $controller->gateway); + } + + public function test_make_resolves_constructor_service_built_by_container_closure(): void + { + app()->singleton(RouteDiShippingService::class, fn () => new RouteDiShippingService(['zone' => 'eu'])); + $route = Route::get('shipping', [RouteDiShippingController::class, 'show']); + + $controller = $this->invoke_route_method($route, 'make', RouteDiShippingController::class); + + $this->assertSame(app()->make(RouteDiShippingService::class), $controller->shipping); + $this->assertSame(['zone' => 'eu'], $controller->shipping->settings); + } + + public function test_make_uses_default_for_scalar_constructor_parameter(): void + { + $route = Route::get('limit', [RouteDiLimitController::class, 'show']); + + $controller = $this->invoke_route_method($route, 'make', RouteDiLimitController::class); + + $this->assertInstanceOf(RouteDiLeafService::class, $controller->leaf); + $this->assertSame(10, $controller->limit); + } + + public function test_make_reuses_controller_bound_as_singleton(): void + { + app()->singleton(RouteDiLimitController::class); + $route = Route::get('limit', [RouteDiLimitController::class, 'show']); + + $first = $this->invoke_route_method($route, 'make', RouteDiLimitController::class); + $second = $this->invoke_route_method($route, 'make', RouteDiLimitController::class); + $this->assertSame($first, $second); - $this->assertSame($first->leaf, $second->leaf); + } + + public function test_make_does_not_share_unbound_dependency_between_controllers(): void + { + $route = Route::get('ping', [RouteDiTestController::class, 'show']); + + $first = $this->invoke_route_method($route, 'make', RouteDiReportController::class); + $second = $this->invoke_route_method($route, 'make', RouteDiExportController::class); + + $this->assertNotSame($first->reports, $second->reports); } public function test_make_detects_circular_dependencies(): void @@ -252,3 +324,118 @@ public function __construct(RouteDiCircularA $dependency) { } } + +class RouteDiCart +{ +} + +class RouteDiCartController +{ + public RouteDiCart $cart; + + public function __construct(RouteDiCart $cart) + { + $this->cart = $cart; + } + + public function add(RequestContract $request, RouteDiCart $cart) + { + return ['constructor' => $this->cart, 'action' => $cart]; + } +} + +interface RouteDiGateway +{ +} + +class RouteDiStripeGateway implements RouteDiGateway +{ +} + +class RouteDiPaymentController +{ + public RouteDiGateway $gateway; + + public function __construct(RouteDiGateway $gateway) + { + $this->gateway = $gateway; + } + + public function show(RequestContract $request) + { + return $request; + } +} + +class RouteDiShippingService +{ + public array $settings; + + public function __construct(array $settings) + { + $this->settings = $settings; + } +} + +class RouteDiShippingController +{ + public RouteDiShippingService $shipping; + + public function __construct(RouteDiShippingService $shipping) + { + $this->shipping = $shipping; + } + + public function show(RequestContract $request) + { + return $request; + } +} + +class RouteDiLimitController +{ + public RouteDiLeafService $leaf; + + public int $limit; + + public function __construct(RouteDiLeafService $leaf, int $limit = 10) + { + $this->leaf = $leaf; + $this->limit = $limit; + } + + public function show(RequestContract $request) + { + return $request; + } +} + +class RouteDiReportService +{ + public RouteDiLeafService $leaf; + + public function __construct(RouteDiLeafService $leaf) + { + $this->leaf = $leaf; + } +} + +class RouteDiReportController +{ + public RouteDiReportService $reports; + + public function __construct(RouteDiReportService $reports) + { + $this->reports = $reports; + } +} + +class RouteDiExportController +{ + public RouteDiReportService $reports; + + public function __construct(RouteDiReportService $reports) + { + $this->reports = $reports; + } +} diff --git a/tests/Unit/TestCase.php b/tests/Unit/TestCase.php index 612d41e..6049e5d 100644 --- a/tests/Unit/TestCase.php +++ b/tests/Unit/TestCase.php @@ -213,7 +213,6 @@ protected function reset_route_state(): void 'routes' => [], 'named_routes' => [], 'group_stack' => [], - 'instances' => [], 'site_router' => null, 'with_site_route' => false, ];