Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Task 001: Add PluginInterceptor Dependency to Container

**Status**: completed
**Depends on**: none
**Retry count**: 0

## Description
Add a `setPluginInterceptor()` setter method to Container, making plugin interception available during resolution without breaking existing code that creates containers without plugins. A setter is required (not a constructor parameter) because `PluginInterceptor` needs a `ContainerInterface` reference in its own constructor, creating a circular dependency that prevents both from being constructed simultaneously.

## Context
- Related files: `packages/core/src/Container/Container.php`
- The `PluginInterceptor` class already exists at `packages/core/src/Plugin/PluginInterceptor.php`
- Container is constructed in `Application::initialize()` — PluginInterceptor cannot be created until Container exists (PluginInterceptor's constructor requires `ContainerInterface`)
- `PluginInterceptor` is a `readonly class` — it takes `ContainerInterface` and `PluginRegistry` in its constructor
- Add a `private ?PluginInterceptor $pluginInterceptor = null` property (not constructor-promoted, not readonly since it is set after construction)
- Add a `public function setPluginInterceptor(PluginInterceptor $interceptor): void` method

## Requirements (Test Descriptions)
- [ ] `it accepts PluginInterceptor via setter method`
- [ ] `it resolves classes without PluginInterceptor when none is set`
- [ ] `it continues to work with PreferenceRegistry when PluginInterceptor is also set`

## Acceptance Criteria
- All requirements have passing tests
- Existing ContainerTest tests continue to pass unchanged
- Code follows code standards
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Task 002: Wrap Resolved Instances with Plugin Proxy

**Status**: completed
**Depends on**: 001
**Retry count**: 0

## Description
Modify `Container::resolve()` to wrap resolved instances with `PluginInterceptor::createProxy()` when a `PluginInterceptor` is available. This is the core change that makes plugins functional. The proxy wrapping should happen for autowired instances and closure bindings, but NOT for pre-registered instances (via `instance()`). Singleton caching must store the proxy, not the raw instance.

## Context
- Related files: `packages/core/src/Container/Container.php`, `packages/core/src/Plugin/PluginInterceptor.php`
- `PluginInterceptor::createProxy()` already returns the raw target when no plugins exist — no extra guard needed
- `resolve()` has three code paths that create instances:
1. The `instances[]` early return (pre-registered and cached singletons) — should NOT be re-wrapped
2. Closure bindings — returns early before end of method, so proxy wrapping must happen INSIDE this block before the return
3. Autowiring — falls through to end of method, proxy wrapping can happen before the return
- **IMPORTANT**: The closure binding code path has an early `return` statement. Proxy wrapping placed "at the end" of `resolve()` will NEVER execute for closure bindings. You must add proxy wrapping in BOTH the closure binding block (before its return) AND the autowiring path (before its return). Alternatively, restructure to have a single exit point.
- The `$id` variable changes during resolution (preferences/bindings may change it). Pass the resolved concrete class name (post-preference/binding `$id`) as `targetClass` to `createProxy()`, NOT `$originalId`
- Pre-registered instances via `instance()` are returned from the `instances[]` cache before proxy wrapping code runs, so they are naturally excluded
- The `PluginInterceptor` is accessed via the setter added in Task 001 (`$this->pluginInterceptor`), which may be null

## Requirements (Test Descriptions)
- [ ] `it wraps resolved instance with plugin proxy when plugins are registered`
- [ ] `it returns raw instance when no plugins are registered for the class`
- [ ] `it caches the proxy as the singleton instance on subsequent resolves`
- [ ] `it wraps closure binding results with plugin proxy`
- [ ] `it does not wrap pre-registered instances from instance() method`
- [ ] `it applies plugin proxy after preference resolution`

## Acceptance Criteria
- All requirements have passing tests
- Existing ContainerTest tests continue to pass
- Code follows code standards
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Task 003: Wire PluginInterceptor into Application Bootstrap

**Status**: completed
**Depends on**: 001, 002
**Retry count**: 0

## Description
Update `Application::initialize()` to create a `PluginInterceptor` and pass it to the Container. This requires reordering the bootstrap sequence slightly: the PluginRegistry must be created before the Container, and the PluginInterceptor needs both the Container and PluginRegistry. Since PluginInterceptor needs the Container reference and Container needs the PluginInterceptor, we need to use a two-phase approach: create Container first, then create PluginInterceptor, then inject it into Container via a setter or by restructuring initialization order.

## Context
- Related files: `packages/core/src/Application.php`, `packages/core/src/Container/Container.php`
- Current bootstrap order in `initialize()`:
1. Container is created with PreferenceRegistry
2. `discoverPlugins()` creates PluginRegistry and populates it
- The problem: `PluginInterceptor` needs BOTH `ContainerInterface` and `PluginRegistry` in its constructor. Container must exist first.
- Solution: Use the setter approach (Task 001 added `setPluginInterceptor()`):
1. Create `PluginRegistry` early (empty, mutable) — before `discoverPlugins()`
2. Create Container as before
3. Create `PluginInterceptor` with the Container and the empty PluginRegistry
4. Call `$container->setPluginInterceptor($interceptor)`
5. `discoverPlugins()` populates the SAME PluginRegistry instance (it's mutable, plugins are added via `register()` and checked at resolve-time)
- The existing `discoverPlugins()` method creates its own `new PluginRegistry()` — this must be changed to use the pre-created instance instead
- Register the PluginInterceptor and PluginRegistry as container instances for other services that may need them

## Requirements (Test Descriptions)
- [ ] `it creates PluginInterceptor and injects it into Container via setter during initialization`
- [ ] `it uses the same PluginRegistry instance for both discovery and interception`
- [ ] `it resolves objects with plugin interception after full initialization`
- [ ] `it creates PluginRegistry before plugin discovery and reuses it for PluginInterceptor`

## Acceptance Criteria
- All requirements have passing tests
- Existing ApplicationTest tests continue to pass
- Code follows code standards
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Task 004: Integration Test — End-to-End Plugin Interception via Container

**Status**: completed
**Depends on**: 002
**Retry count**: 0

## Description
Create integration tests that prove the full plugin lifecycle works through the container: register plugins in PluginRegistry, resolve a target class from Container, call a method, and verify before/after plugins fire. These tests validate that the system works as a whole, not just individual pieces.

## Context
- Related files: `packages/core/tests/Unit/Container/ContainerTest.php` (add tests here alongside existing container tests)
- Patterns to follow: Existing `PluginInterceptorTest.php` fixtures and `ContainerTest.php` style
- Tests should manually wire components: create Container, create PluginRegistry, create PluginInterceptor (with Container + Registry), call `$container->setPluginInterceptor($interceptor)`, register plugins in the registry, then resolve and call methods
- Do NOT rely on Application bootstrap — these are unit/integration tests of the Container+Plugin wiring
- Use `describe('plugin interception')` block to group these tests
- For the preference + plugin test: register a preference (`InterfaceX` -> `ConcreteY`), register a plugin targeting `ConcreteY`, resolve `InterfaceX` from container, call a method, verify plugin fires

## Requirements (Test Descriptions)
- [ ] `it fires before plugin when calling method on container-resolved object`
- [ ] `it fires after plugin when calling method on container-resolved object`
- [ ] `it passes modified arguments from before plugin to target method`
- [ ] `it passes modified result from after plugin back to caller`
- [ ] `it fires plugins on preference-resolved objects`
- [ ] `it returns same proxied singleton on repeated resolves`

## Acceptance Criteria
- All requirements have passing tests
- Tests demonstrate real end-to-end behavior (not mocked)
- Code follows code standards
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Task 005: Prevent Plugins from Targeting Plugin Classes

**Status**: completed
**Depends on**: none
**Retry count**: 0

## Description
Add validation to `PluginRegistry::register()` that prevents registering a plugin whose target class is itself a plugin (has the `#[Plugin]` attribute). This enforces the architectural rule that plugins modify business logic on service classes, not other plugins. If someone needs to alter a plugin's behavior, they should use a Preference to replace the plugin class entirely.

## Context
- Related files: `packages/core/src/Plugin/PluginRegistry.php`, `packages/core/src/Exceptions/PluginException.php`
- PluginRegistry already validates conflicting sort orders and duplicate hooks during `register()`
- Use reflection to check if the target class has a `#[Plugin]` attribute — this is order-independent (doesn't matter which plugin is registered first)
- Add a new static factory method to `PluginException` for this case (e.g., `PluginException::cannotTargetPlugin()`)
- The error message should be helpful: explain that plugins cannot target other plugin classes, and suggest using a Preference to replace the plugin instead

## Requirements (Test Descriptions)
- [ ] `it throws PluginException when registering a plugin that targets another plugin class`
- [ ] `it includes helpful message suggesting Preference as alternative`
- [ ] `it allows registering plugins that target non-plugin classes`

## Acceptance Criteria
- All requirements have passing tests
- Error message follows Marko's "loud errors with helpful messages" principle
- Code follows code standards
95 changes: 95 additions & 0 deletions .claude/plans/container-plugin-integration/_devils_advocate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# Devil's Advocate Review: container-plugin-integration

## Critical (Must fix before building)

### C1. Circular dependency makes Option C impossible (Task 003)

Task 003 describes Option C as "cleanest": create `PluginRegistry` and `PluginInterceptor` before Container, pass to Container constructor. But `PluginInterceptor`'s constructor requires `ContainerInterface`:

```php
readonly class PluginInterceptor
{
public function __construct(
private ContainerInterface $container,
private PluginRegistry $registry,
) {}
}
```

You cannot create `PluginInterceptor` before `Container` exists, because it needs the Container. And you cannot pass `PluginInterceptor` to Container's constructor because it does not exist yet.

**Fix**: Option A (setter method) is the only viable approach. Task 001 must add a `setPluginInterceptor()` method to Container. Task 003 must use Option A: create Container first, create PluginInterceptor with the Container reference, then call `$container->setPluginInterceptor($interceptor)`. This also means Container cannot use `readonly` on the `$pluginInterceptor` property.

### C2. Closure binding early return bypasses end-of-method proxy wrapping (Task 002)

Task 002 says "proxy wrapping should happen at the end of `resolve()`, just before returning." But the closure binding code path returns early at line 132:

```php
if ($binding instanceof Closure) {
$instance = $binding($this);
if (isset($this->shared[$originalId])) {
$this->instances[$originalId] = $instance;
}
return $instance; // <-- EARLY RETURN, skips any code at end of method
}
```

Any proxy wrapping placed "at the end" of `resolve()` will never execute for closure bindings. The plan's architecture note ("all code paths get wrapped") is incorrect for the current structure.

**Fix**: Task 002 must explicitly handle proxy wrapping in BOTH the closure binding return path AND the autowiring return path. The task description must specify that proxy wrapping code needs to be added in two places (or the resolve method must be restructured to have a single exit point).

### C3. Task 004 missing dependency on Task 003 (Task 004)

Task 004 says "Depends on: 002" but its description says "These tests validate that the system works as a whole." The integration tests that wire `PluginRegistry` + `PluginInterceptor` + `Container` together require the setter/wiring mechanism from Task 003. Without Task 003, there is no way to inject `PluginInterceptor` into Container (since the constructor approach requires the circular dependency fix).

Actually, on closer inspection, Task 004 could manually construct all pieces in the test without needing Application wiring. The tests create a Container, create a PluginInterceptor with that Container, and inject it via the setter from Task 001. This works without Task 003. However, the dependency should still be explicit since Task 004 needs the setter method from Task 001 AND the proxy wrapping logic from Task 002.

**Fix**: Task 004's dependency is correct (depends on 002, which depends on 001), but the task context section must clarify that tests should manually wire components using the setter from Task 001, not rely on Application bootstrap.

## Important (Should fix before building)

### I1. Line number references will be stale (Task 002)

Task 002 references specific line numbers from the current Container.php (line 160-191, line 126-132, line 106-108). After Task 001 adds a new constructor parameter and potentially a setter method, all these line numbers will shift. Workers building Task 002 will be confused by wrong line references.

**Fix**: Replace line number references with descriptive code path names: "the `instances[]` early return", "the closure binding block", "the autowiring block", "the singleton caching block."

### I2. Setter method violates `readonly` expectations (Task 001)

The `PluginInterceptor` property on Container cannot be `readonly` since it must be set after construction via a setter. Container class itself is not `readonly` (it has mutable `$shared`, `$instances`, `$bindings` arrays), so this is fine structurally. But the task must explicitly state this property is nullable and set via setter, not via constructor.

**Fix**: Task 001 must specify: add a `private ?PluginInterceptor $pluginInterceptor = null` property (not constructor-promoted) and a `setPluginInterceptor(PluginInterceptor $interceptor): void` method. Add a test requirement: "it accepts PluginInterceptor via setter method."

### I3. No test for plugin on interface-requested class via preference (Task 004)

The plan's architecture notes discuss preferences + plugins, but the test requirement "it fires plugins on preference-resolved objects" is vague. The specific scenario to test: register a preference `InterfaceA` -> `ConcreteB`, register a plugin targeting `ConcreteB`, resolve `InterfaceA` from container, call a method, verify plugin fires. This is the critical cross-cutting test.

**Fix**: Add explicit setup instructions to Task 004 for the preference + plugin test case.

### I4. `createProxy` targetClass parameter needs clarification (Task 002)

When preferences are in play, `$id` changes from the original interface to the concrete class during resolution. The `createProxy()` call should use `$id` (the resolved concrete class), not `$originalId` (the requested interface). The plan's architecture note says "plugins target concrete classes with public methods" which is correct, but Task 002 doesn't explicitly specify which variable to pass as `targetClass`.

**Fix**: Task 002 requirements should explicitly state: "pass the resolved concrete class name (post-preference `$id`) as `targetClass` to `createProxy()`, not the original requested `$id`."

## Minor (Nice to address)

### M1. No test for plugin class resolution causing recursion

The plan mentions infinite recursion risk under "Risks & Mitigations" but no test validates that resolving a plugin class from within `PluginProxy::__call()` doesn't cause infinite proxy wrapping. If a plugin class itself has plugins registered against it, the Container would try to proxy-wrap the plugin, which would create another proxy, etc. This is an edge case but worth a test.

### M2. `PluginProxy` uses `__call()` which code standards forbid

Code standards say "No magic methods -- Avoid `__get`, `__set`, `__call`, `__callStatic`." `PluginProxy` relies on `__call()` by design. This is pre-existing and out of scope for this plan, but worth noting the tension.

### M3. No `bind()` method in ContainerInterface

Container has `bind()` as a public method but it's not on `ContainerInterface`. This means code that only has `ContainerInterface` (like `PluginProxy` which holds `ContainerInterface`) cannot call `bind()`. Not an issue for this plan specifically, but worth noting.

## Questions for the Team

1. **Should `setPluginInterceptor()` be on `ContainerInterface`?** Currently, the interface only has `singleton()`, `instance()`, `call()`, and the PSR methods. If the setter is only called during Application bootstrap, it could stay on the concrete `Container` class only. But if other code needs to set interceptors, it should be on the interface.

2. **Should proxy wrapping apply to `call()` resolved dependencies?** Currently only `resolve()` is being modified. But `call()` also resolves dependencies via `$this->resolve()`. Since `resolve()` is the internal method and `call()` delegates to it, this should be covered. But worth confirming.

3. **Should there be a guard against double-proxying?** If someone calls `container->get(Foo::class)` and stores the proxy, then later registers it via `container->instance('foo', $proxy)`, and another resolve path hits it -- could it get double-wrapped? The `instance()` exclusion prevents this for pre-registered instances, but worth thinking about.
Loading