Skip to content

Introduce Argument Augmenter#26

Draft
henriquemoody wants to merge 1 commit into
Respect:mainfrom
henriquemoody:augment
Draft

Introduce Argument Augmenter#26
henriquemoody wants to merge 1 commit into
Respect:mainfrom
henriquemoody:augment

Conversation

@henriquemoody

Copy link
Copy Markdown
Member

Unlike the resolver, which completes a call by padding gaps with defaults or null, the augmenter keeps the given arguments authoritative and only adds container services for parameters left unfilled. This suits factories that pass user input straight to a constructor.

Types listed as unresolvable are never looked up in the container, which keeps value-like classes (clocks, dates) from being served as frozen services.

Unlike the resolver, which completes a call by padding gaps with
defaults or null, the augmenter keeps the given arguments authoritative
and only adds container services for parameters left unfilled. This
suits factories that pass user input straight to a constructor.

Types listed as unresolvable are never looked up in the container,
which keeps value-like classes (clocks, dates) from being served as
frozen services.
@alganet

alganet commented Jun 12, 2026

Copy link
Copy Markdown
Member

Unlike the resolver, which completes a call by padding gaps with defaults or null

You can pass overrides to it as well, which I believe solve many of the use cases for the augment. It's not only defaults and nulls.

I ran your README example (Notifier with ['slack']) through the existing resolve(): it keeps the caller's argument, pulls the same Mailer instance from the container, and builds an identical object. The "padding" only changes the shape of the returned array, not the call it produces.


What the new augmenter does is:

  • Introduces unresolvableTypes, which is something we would also want in resolve (it's a new global feature). I understand we need this.
  • Allows variadics, which is nice, but introduces many complications. Do we need it though?
  • Preserves explicit nulls, whichs is perhaps a bug (isset vs array_key_exists).

My suggestion:

  • Introduce unresolvableTypes to the Resolver as an optional parameter. Everyone gets that benefit.
  • Fix the explicit-null case in resolve() itself: array_key_exists() instead of isset(), gated on $param->allowsNull() so a non-nullable service param still gets injected (keeps BC with how Respect\Rest uses params).
  • For variadics, make resolve() return a fully positional list (array_values() + leftover args) when $reflection->isVariadic(), keeping the name-keyed shape for everything else. That's the only shape PHP's splat accepts (int keys after string keys fatal with "Cannot use positional argument after named argument during unpacking", the augmenter's output hits this too). I tested this variant against Rest's full suite: green, and it fixes a real bug there.

With those three changes in Resolver, I don't think the parallel Augmenter interface + ContainerAugmenter + reflection cache holds.


Draft diff for Resolver:

diff --git a/src/Resolver.php b/src/Resolver.php
index f115be1..bd855a6 100644
--- a/src/Resolver.php
+++ b/src/Resolver.php
@@ -19,8 +19,10 @@ use ReflectionNamedType;
 use ReflectionParameter;
 
 use function array_key_exists;
+use function array_values;
 use function assert;
 use function count;
+use function in_array;
 use function is_a;
 use function is_array;
 use function is_object;
@@ -35,8 +37,11 @@ use function str_contains;
  */
 final readonly class Resolver
 {
-    public function __construct(private ContainerInterface $container)
-    {
+    /** @param array<class-string> $unresolvableTypes */
+    public function __construct(
+        private ContainerInterface $container,
+        private array $unresolvableTypes = [],
+    ) {
     }
 
     /**
@@ -61,13 +66,18 @@ final readonly class Resolver
             $paramName = $param->getName();
             $typeName = self::typeName($param);
 
-            if ($typeName !== null && isset($arguments[$argIndex]) && $arguments[$argIndex] instanceof $typeName) {
+            if (
+                $typeName !== null
+                && array_key_exists($argIndex, $arguments)
+                && ($arguments[$argIndex] instanceof $typeName
+                    || ($arguments[$argIndex] === null && $param->allowsNull()))
+            ) {
                 $resolvedArgs[$paramName] = $arguments[$argIndex++];
 
                 continue;
             }
 
-            if ($typeName !== null && $this->container->has($typeName)) {
+            if ($typeName !== null && $this->resolvable($typeName)) {
                 $resolvedArgs[$paramName] = $this->container->get($typeName);
 
                 continue;
@@ -82,6 +92,13 @@ final readonly class Resolver
             }
         }
 
+        if ($reflection->isVariadic()) {
+            $resolvedArgs = array_values($resolvedArgs);
+            while ($argIndex < $argCount) {
+                $resolvedArgs[] = $arguments[$argIndex++];
+            }
+        }
+
         return $resolvedArgs;
     }
 
@@ -113,7 +130,7 @@ final readonly class Resolver
 
             $typeName = self::typeName($param);
 
-            if ($typeName !== null && $this->container->has($typeName)) {
+            if ($typeName !== null && $this->resolvable($typeName)) {
                 $resolvedArgs[$paramName] = $this->container->get($typeName);
 
                 continue;
@@ -165,6 +182,12 @@ final readonly class Resolver
         return false;
     }
 
+    /** @param class-string $typeName */
+    private function resolvable(string $typeName): bool
+    {
+        return !in_array($typeName, $this->unresolvableTypes, true) && $this->container->has($typeName);
+    }
+
     /** @return class-string|null */
     private static function typeName(ReflectionParameter $param): string|null
     {

WDYT? This would improve the already existing resolver, keeping BC with the Respect\Rest use case and possibly solving your case (if I didn't miss anything).

return $this->augmentableParametersCache[$cacheKey] = $parameters;
}

private static function createCacheKey(ReflectionFunctionAbstract $reflection): string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you run benchmarks? Claude often recommends caching reflection, but in PHP 8.5, he's wrong. Reflection is fast now, and caching it often decreases performance in some cases.

There are legitimate cases though (such as the attribute/template thing we did on Validation which involves multiple reflection calls that coalesce in a single cache).

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.

2 participants