[11.x] fix: container resolution order when resolving class dependencies

[calebdw]

Hello!

Changes

Currently, when resolving a class with with dependencies, the container only falls back to the parameter default value if the class dependency failed to resolve. However, this is not intuitive---if I specify a default value for a parameter then I would expect that default value to be used (if I have not explicitly bound that parameter class to the container). This updates the container resolution logic to use the default value for a dependency (when available) before trying to resolve a class instance. The container class dependency logic flow now looks like:

1. check for explicitly bound instance
2. check for default value
3. attempt to make instance
4. if 3) fails and variadic then return array

For example, consider resolving the following class from the container:

class Foo
{
    public function __construct(
        // non-nullable with no default, I would expect this to be resolved with a class instance
        public Carbon $date,
        // nullable with no default, I would expect this to be resolved with a class instance
        public ?Carbon $noDefault,
        // nullable with default specified, I would expect this to be resolved with null
        public ?Carbon $default = null,
    ) {}
}
$foo = app()->make(Foo::class);

// before
$foo->date instanceof Carbon; // expected
$foo->noDefault instanceof Carbon; // expected
$foo->default instanceof Carbon; // NOT expected or intuitive!!

// now
$foo->date instanceof Carbon; // same 
$foo->noDefault instanceof Carbon; // same
$foo->default === null; // changed, the default is now used as expected

app()->bind(Carbon, fn () => now());
$foo = app()->make(Foo::class);
$foo->default instanceof Carbon; // expected as I explicitly bound the class

Note

This does change resolution behaviour, but this shouldn't break existing classes because they should already be setup to handle the default value if it's been specified. However, I can target 12.x if this is considered a risky change.

Motivation

I would consider this particular container nuance to be a bug and this has bitten us several times (usually when dealing with Model or Carbon instances) resulting in lengthy debugging efforts for seemingly obscure problems.

Most recently, this reared its head when using a scheduled, dispatched job. This particular job is designed to be executed early in the morning and it aggregates results for the previous day. We do allow a date to be passed into the constructor so the same job can be manually dispatched for a particular date if necessary---it is considered an override when anything other than null is passed in.

class ScheduledJob implements ShouldQueue
{
    use Dispatchable;
    use InteractsWithQueue;
    use Queueable;

    public function __construct(protected ?CarbonImmutable $date = null) 
    {
        $this->date ??= now()->subDay()->toImmutable();
    }

    public function __invoke(): void {/* ... */}
}
Schedule::job(ScheduledJob::class)->cron('...'); // the scheduled jobs fail
ScheduleJob::dispatch(); // dispatching manually works

However, this job fails when executed via the scheduler every morning because the Scheduler uses the container to make the job class:

framework/src/Illuminate/Console/Scheduling/Schedule.php

Lines 165 to 166 in a296e81

	return $this->call(function () use ($job, $queue, $connection) {
	$job = is_string($job) ? Container::getInstance()->make($job) : $job;

and the container ignores the default value and instead constructs the job with a Carbon instance = now---the logic to handle the default case is never executed. This works just fine as intended when dispatching the job with ScheduledJob::dispatch() (because of which, this was a tricky bug to troubleshoot). I would expect the job to have the same behaviour regardless if it was dispatched manually via ::dispatch() or automatically via the scheduler.

I know I can use

Schedule::job(new ScheduledJob())->cron('...');

to schedule the job with a class instance, however:

1. we have many scheduled jobs and newing them all up would incur unnecessary overhead on every console invocation
2. this would fail with schedule:work because the constructor would be invoked when the app is booted and not right before the job is dispatched to the queue---resulting in a stale $date property

Note

I recognize that there are many ways to work around this current behaviour, but the crux of the issue is that the resolution logic is not intuitive and ignores the developer provided default. This issue has continued to pop up in various locations in the app when using the container to resolve classes with dependencies that have default values.

Thanks!

[taylorotwell]

I don't think there is any way we could merge this into 11.x. It's such a subtle change that could cause a lot of headache if applications are built to expect this behavior.