[9.x] Log facade to accept any number of arguments of any type

[dusterio]

Log facade is one of the crucial elements of Laravel development – this is how you get feedback when things go wrong, as well as when things go well or when you just want to get some extra visibility over your app.

Laravel framework binds LogManager class under the 'log' key in its container and currently it implements a PSR LoggerInterface:

public function emergency(string|\Stringable $message, array $context = []): void;

Where we can pass a message as well as a context - any supplemental data that we need. If we refer to the PSR documentation:

1.3 Context
Every method accepts an array as context data. This is meant to hold any extraneous information that does not fit well in a string. The array can contain anything. Implementors MUST ensure they treat context data with as much lenience as possible. A given value in the context MUST NOT throw an exception nor raise any php error, warning or notice.

Imagine a scenario like this:

try {
    $details = $api->request(); // $api is some kind of a third-party SDK
} catch (ClientException $e) {
    // $e->getResponse() might have a nice JSON error message but might have a CDN's HTML error or something else
    Log::emergency("Oh no, we got a failed API request!", json_decode($e->getResponse()->getContent()));
}

A very usual situation indeed – somewhere in our code we have a failing third-party integration and we want to log the output from that API, perhaps there is something useful in there that's going to help us troubleshoot this later.

The problem here is that the second parameter of Log::emergency() call HAS to be an array and if json_decode() doesn't return an array for whatever reason, our Laravel application is going to crash!

Illuminate\Log\LogManager::emergency(): Argument #2 ($context) must be of type array, bool given, called in vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php on line 338 in vendor/psy/psysh/src/Exception/TypeErrorException.php on line 53.

Oh, that's not nice! Remember JavaScript's console.log() command? You can pass anything to it and it's never going to kill your application:

console.log('plain message')
// plain message
console.log('plain message', 123, [])
// plain message 123 []

The idea is simple - because we use the logging capability to troubleshoot things, the last thing we want is for the application to crash BECAUSE of the logging. I believe I should be able to pass just about anything to Log facade and it should just work in one capacity or another.

This pull-request is one of the possible implementations where I removed the "array" type from the method signatures in LogManager class. This doesn't violate the LoggerInterface contract that the class implements. Moreover, it's been done before if we look closer in the Logger class:

    /**
     * @param  \Illuminate\Contracts\Support\Arrayable|\Illuminate\Contracts\Support\Jsonable|\Illuminate\Support\Stringable|array|string  $message
     * @param  array  $context
     * @return void
     */
    public function info($message, array $context = []): void
    {
        $this->writeLog(__FUNCTION__, $message, $context);
    }

Compare it with the actual LoggerInterface signature:

    /**
     * @param string|\Stringable $message
     * @param mixed[] $context
     *
     * @return void
     */
    public function info(string|\Stringable $message, array $context = []): void;

As we can see the string|\Stringable type was removed from the signature so that Laravel can use the formatMessage() method later to display Stringable and Jsonable objects nicely.

I propose we do the same with the $context with the same goal - make it a bit more flexible and convenient.

With the updates proposed, I can do stuff like this:

Log::emergency("Hello", $results, 'Another message');

And it just all goes to the log driver, to the logs. I don't have to worry about the number of arguments I pass or the types of arguments I pass (which are hard to predict sometimes). I never have to worry about Log throwing an error again.

Any feedback, opinions, and alternative implementations are welcome! :)

[ankurk91]

The idea is good but it is breaking change.

[taylorotwell]

This is something that would need to be considered for Laravel 10.x. But, I don't think it's really that hard to just pass an array so don't totally see the issue.

[dusterio]

Just in case anybody finds this PR in the future and has the same pain as we do, we just quickly extracted this into a package: https://github.com/TixelRocks/laravel-safe-logger

While I agree that it's trivial to just always make sure the first parameter is a string and the second parameter is an array, I also strongly believe that logging should never crash any software app - built in any language or framework. Logging is the last resort kind of thing when things go bad, it really doesn't make any sense for logging to be the cause of a problem itself.