Injecting a specific logger using Dependency Injection from AppServiceProvider creates bugs within the framework error handler logger

[hilnius]

Laravel Version

12.9.1

PHP Version

8.3.15

Database Driver & Version

No response

Description

When dynamically injecting a Logger in a class using dependency injection, the framework application container builds other dependencies.
If one of these dependencies has a problem in its constructor (like a deprecation warning), then when trying to get the deprecation logger, the error handler asks to the container for a LogManager but ends up with a Logger, which makes the call to $logger->channel fail because a Logger has no channel method.

HandleExceptions.php :

       try {
            // HERE, asking for a LogManager, but getting a Logger instead
            $logger = static::$app->make(LogManager::class);
        } catch (Exception) {
            return;
        }

        $this->ensureDeprecationLoggerIsConfigured();

        $options = static::$app['config']->get('logging.deprecations') ?? [];

        // HERE, the call to channel fails and throws an error
        with($logger->channel('deprecations'), function ($log) use ($message, $file, $line, $level, $options) {
            if ($options['trace'] ?? false) {
                $log->warning((string) new ErrorException($message, 0, $level, $file, $line));
            } else {
                $log->warning(sprintf('%s in %s on line %s',
                    $message, $file, $line
                ));
            }
        });

I don't know why this happens as it was too complex for me to dig deeper inside the inner workings of the Application Container, but for sure this looks like a bug.

Steps To Reproduce

clone & run : (GET '/')
https://github.com/hilnius/laravel-container-logger-issue

specific commit with the diff (based on a brand new 'laravel new' repository)
hilnius/laravel-container-logger-issue@e2903ff

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[dvd-b]

@hilnius
I believe the cleanest and least invasive solution is to add an explicit and priority binding for LogManager in the service provider:

// In AppServiceProvider.php, register() method
public function register(): void
{
    // Explicit binding to ensure correct resolution of LogManager
    $this->app->bind(LogManager::class, function($app) {
        return $app['log'];
    });
    
    // The existing conditional binding
    $this->app->when(InjectedClass::class)
        ->needs(LoggerInterface::class)
        ->give(fn () => Log::channel('single'));
}

This solution ensures that:

1. When LogManager::class is explicitly requested, the container always returns the correct instance of LogManager
2. The conditional binding for InjectedClass continues to work as expected

[rodrigopedra]

@crynobone issue seems to be (considering the reproduction repo provided by @hilnius):

• When building the InjectedClass, it gets pushed to the Container@buildStack property, while building its constructor's dependencies
• While building the SecondClass dependency, the SecondClass is not pushed into the Container@buildStack property
• When the deprecation happens, and the framework wants to log it, it tries to build a 'log' instance
• As the last element of Container@buildStack is still the InjectedClass, and not the SecondClass, the Container@getContextualConcrete() call within Container@resolve() returns the factory registered in the AppServiceProvider
• So instead of resolving the global log singleton, it uses the contextual factory registered to the InjectedClass

On my local install, this change to the Container@make() method made the reproduction repository work:

public function make($abstract, array $parameters = [])
{
    if (class_exists($abstract) && end($this->buildStack) !== $abstract) {
        $pushedToStack = true;
        $this->buildStack[] = $abstract;
    }

    try {
        return $this->resolve($abstract, $parameters);
    } finally {
        if ($pushedToStack ?? false) {
            array_pop($this->buildStack);
        }
    }
}

yes, changed manually on the vendor directory, for easiness of testing

The idea is to always push a resolving dependency to the Container@buildStack property, before starting resolving it.

But then, I tested on my local laravel/framework fork, and had one test case failing. A single one, but still.

The failed test was about a mismatch on an expected exception message, that ended having duplicated classes on the Container@buildStack property. (see: Illuminate\Tests\ContainerContainerTest@testBindingResolutionExceptionMessageIncludesBuildStack())

As I think the Container is a highly sensitive component, and would require reviewing a lot of execution paths, I can't work on a PR due to lack of time this week.

An option could be making a class for the build stack which implements ArrayAccess and prevents duplicates being pushed to the end of the stack.

But I thought on sharing my findings, in case it could help someone to work on this.

---

Note:

I tried flushing the Container@buildStack when an exception is thrown, before trying to resolve any other thing from the container. But the deprecation is an error, not an exception.

Thus, the try/catch block I added does not catch it, and it goes directly to the ExceptionHandler, as it registers a global error handler within PHP.

Upon trying to log the deprecation error, the container still has the InjectedClass class on the end of its Container@buildStack property, and well, it happens the same as explained above.

[rodrigopedra]

As an additional note, I have the very same problem on a project of mine.

I could workaround it by injecting the LogManager instead of the LoggerInterface, and calling the LogManager@channel() method on each class' constructor.

This project in particular needed 3 different loggers due to different auditing requirements. Many action classes require one of them.

I don't like the hardcoded channel on each class' constructor, but by the time I was building the project, it took me so much time fighting the framework that I lacked the motivation to investigate further by then.

By the way, I had that problem on Laravel 8. This app is already ported to Laravel 12, and the issue is still the same.

It would be very nice if this gets resolved.

[hilnius]

@rodrigopedra thanks for the detailed investigation.

As I think the Container is a highly sensitive component, and would require reviewing a lot of execution paths, I can't work on a PR due to lack of time this week.

this is why I don't really feel like I can be of help here - it's burried deep in the framework & I don't have the required time to understand the foundations of how laravel's dependency injection system works :) but glad you did & explained the original issue.

Also as a side note - I was able to fix my issue by fixing the deprecation itself (of course, if there's no attempt to log, there's no problem) - however this is still worth a root fix imho.

@dvd-b thanks for the workaround but I don't think that users of the framework want to have to add such code to their default AppServiceProvider ?