Skip to content

An unresolvable parameter takes its declared default - #1

Merged
alexstandiford merged 3 commits into
mainfrom
fix/honor-declared-defaults
Aug 23, 2026
Merged

An unresolvable parameter takes its declared default#1
alexstandiford merged 3 commits into
mainfrom
fix/honor-declared-defaults

Conversation

@alexstandiford

Copy link
Copy Markdown
Contributor

The container handed every parameter it could not autowire the ReflectionParameter object itself. PHP then coerced that object to the declared type, so an optional string arrived as the text Parameter #2 [ <optional> string $schemaPath = ... ] and the class worked from debris instead of from its default.

This is not theoretical. A DI-resolved ConfigLoader in the Novatorius orchestrator took that string as its JSON-schema path, every schema check threw past the handler that was supposed to catch it, and POST /v1/fleet/ladders answered a blind 500. Arena promotions failed live on 2026-08-23.

What changed

Resolution now runs one rule per constructor parameter:

  1. A class or interface type gets autowired, as before.
  2. Anything the container cannot autowire falls back to the declared default. That covers scalars, arrays, callables, untyped parameters, nullable class types with no binding, and unions.
  3. A parameter with neither a resolvable type nor a default is a wiring mistake, so it still throws DiException. The message now names the parameter and the class, and chains the resolution failure that led there.

Resolution still wins over the default, so a nullable dependency that is bound keeps arriving as the bound instance.

Three smaller faults went with it:

  • An untyped parameter called isBuiltin() on a null type and fataled.
  • A union or intersection type hit the same call on a type object that has no such method.
  • A variadic tail collected reflection debris as its first argument. It now stays empty.

Tests

The repository had no test setup, so this adds the phpunit.xml, bootstrap, and CI workflow the sibling packages use. Sixteen tests cover each parameter shape, including the loader shape that broke live.

Twelve of the sixteen fail against the old resolver, and the failure text is the defect verbatim:

Failed asserting that file "Parameter #2 [ <optional> string $schemaPath = '.../tests/Fixtures/schema.json' ]" exists.

The container handed every parameter it could not autowire the
ReflectionParameter object itself. PHP then coerced that object to the
declared type, so an optional string arrived as the text
"Parameter #2 [ <optional> string $schemaPath = ... ]" and the class
worked from debris instead of from its default. Downstream, a
DI-resolved ConfigLoader in the orchestrator validated against a schema
"path" that was not a path, and the ladders API answered a blind 500.

Resolution now runs one rule per parameter. A class or interface type
gets autowired as before. Anything the container cannot autowire falls
back to the declared default, which covers scalars, arrays, callables,
untyped parameters, nullable class types, and unions. A parameter with
neither a resolvable type nor a default is a wiring mistake, so it still
throws DiException, now naming the parameter and the class.

Three smaller faults went with it. An untyped parameter called
isBuiltin() on a null type and fataled. A union or intersection type hit
the same call on a type object that has no such method. A variadic tail
collected reflection debris as its first argument, and now stays empty.

The repository had no test setup, so this adds the phpunit.xml, test
bootstrap, and CI workflow the sibling packages use. The suite covers
each parameter shape, and twelve of its sixteen tests fail against the
old resolver.
The php-actions runner downloaded its own PHPUnit 13, which needs PHP
8.4 and ignores the 9.6 the lockfile pins, so the job failed before it
ran a test. Set up PHP directly, install from the lockfile, and run the
vendored binary across 8.1, 8.2 and 8.3.
The lockfile's php-cs-fixer pulls Symfony 7.4, which needs PHP 8.2, so
the 8.1 job could not install before it could test.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2dab3ab720

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/Container.php
Comment on lines +183 to +186
} catch (DiException | ReflectionException $e) {
// Nothing is bound for this type. Try the next candidate, and
// then the declared default.
$failure = $e;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Propagate failures from explicitly bound dependencies

When an optional class/interface parameter has an explicit binding whose factory or dependency graph throws DiException, this catch treats the failure as though the type were merely unbound and silently supplies the parameter default. For example, a failing factory for a nullable database or validator dependency now produces an object with null instead of aborting startup, hiding the configuration error and allowing degraded execution. Only genuine absence of a resolvable binding should trigger the default; failures from an explicit binding should propagate.

Useful? React with 👍 / 👎.

Comment thread lib/Container.php
Comment on lines +225 to +229
if ($type instanceof ReflectionUnionType || $type instanceof ReflectionIntersectionType) {
$names = [];
foreach ($type->getTypes() as $member) {
foreach ($this->autowirableTypes($member) as $name) {
$names[] = $name;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Resolve intersection types as a single constraint

Do not flatten ReflectionIntersectionType like a union: when a constructor requires A&B and A is bound to an object implementing only A, this returns that first object immediately, and newInstanceArgs() then throws a TypeError even if another binding could satisfy the complete intersection. Intersection candidates must be accepted only when the resolved object implements every member, rather than treating the members as alternatives.

Useful? React with 👍 / 👎.

@alexstandiford
alexstandiford merged commit 42be402 into main Aug 23, 2026
4 checks passed
@alexstandiford
alexstandiford deleted the fix/honor-declared-defaults branch August 23, 2026 22:17
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.

1 participant