[Cache] Error message does not contain cache-adapter placeholder, so it will not populated

[holtkamp]

Symfony version(s) affected

6.2.0

Description

When a CacheAdapter logs an error using CacheItem::log(), an array with context information is provided. Often this array also include the 'cache-adapter' => get_debug_type($this) to handover the name of the cache adapter to the error message, which is useful when multiple CacheAdapters are involved.

https://github.com/symfony/cache/blob/64cb231dfb25677097d18503d1ad4d016b19f19c/Adapter/AbstractAdapter.php#L182-L183

However, since the message itself does not contain the cache-adapter placeholder, this information is not included in the eventual error message.

https://github.com/symfony/cache/blob/64cb231dfb25677097d18503d1ad4d016b19f19c/CacheItem.php#L155-L161

When a PSR Logger is assigned to the CacheAdapter, this might not be a big problem (since the context is handed over and used in another way). But when trigger_error is used, this information is really lost.

How to reproduce

Have a CacheAdapter log a messages using CacheItem::log() and check whether the name of the CacheAdapter end up in the error message of the logs.

Possible Solution

Include {cache-adapter} placeholder in the error message, for example:

$message = sprintf('Failed to save key "{key}" of type %s%s to CacheAdapter "{cache-adapter}"', $type, $e instanceof \Exception ? ': '.$e->getMessage() : '.');
CacheItem::log($this->logger, $message, ['key' => substr($id, \strlen($this->namespace)), 'exception' => $e instanceof \Exception ? $e : null, 'cache-adapter' => get_debug_type($this)]);

Additional Context

No response

[nicolas-grekas]

Maybe we could add the adapter to the message when trigger_error is used?

if (isset($context['cache-adapter']) && !isset($replace['{cache-adapter}'])) {
	$message .= sprintf(' (from "%s")', $context['cache-adapter']);
}

or something like that?
Would you be up for a PR? This should target branch 6.3 as it would be an improvement.

[holtkamp]

This might be an approach indeed..

But would changing the error message not be more pragmatic / simpler / less magic?

Coming month quite busy... so will be 2023Q1 earliest

[nicolas-grekas]

I personally like compact messages :)
That's not a strong requirement though.
When you can of course!

[carsonbot]

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

[carsonbot]

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

[holtkamp]

@carsonbot sorry, did not have time yet to dive into this. Anyone else?

[carsonbot]

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

[carsonbot]

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

[holtkamp]

Hi @carsonbot, thanks for the reminder.

No nothing has been done from my side due to lack of time.

I still see think that injecting the name of the CacheAdapter in the error message would be useful for people that use a chain of multiple CacheAdapters.