Stop assuming LogLevel::* levels, because \Psr\Log\LoggerInterface::log()does not require it either

[wimleers]

👋

We're looking into adopting this in Drupal over at https://www.drupal.org/project/drupal/issues/3321905.

However, we ran into a blocker. We have to do write code like

$this->assertTrue($logger->hasRecordThatContains('[name] => AAA 8.x-5.x', (string) RfcLogLevel::ERROR));

… because this package assumes the level is always a string, specifically one of LogLevel::* (although that's not enforced, but just prescribed by the documentation).

But \Psr\Log\LoggerInterface::log() prescribes only mixed $level. And the Drupal logging implementation (\Drupal\Core\Logger\LoggerChannel::log()) stores integers, not strings:

    if (is_string($level)) {
      // Convert to integer equivalent for consistency with RFC 5424.
      $level = $this->levelTranslation[$level];
    }

… which means that we unfortunately cannot actually use colinodell/psr-testlogger, except in the awkward way shown above. It also means that we can never use the nicer accessors such as hasErrorThatContains() etc.

Therefore I propose to optionally allow specifying a mapping of LogLevel::* strings to alternative values (in our case: RFC 5424-compliant integers). This would present no BC break; it would be entirely optional.

[wimleers]

PR opened!

Test coverage should be expanded, but I first would like a +1 from you on the approach 😊 🙏

[colinodell]

👋 I'm very much in favor of removing this faulty assumption from this library, even if that does require a major version bump. Using a level map makes sense to me, so I think your PR is on the right track!

I do wonder if we should also use this opportunity to fully standardize how we use $level throughout this class, instead of assuming it will always be a string. Widening the parameters to support more than just string would be backward-compatible. But changing the keys in public array properties might technically break BC. And we also can't truly use mixed for $recordsByLevel keys since PHP only supports strings and ints as keys... so maybe widening this to only support string and int keys would be sufficient?

Open to your thoughts here! I'll also leave a couple other notes on your PR :)

[jonathanjfshaw]

Technically floats, bools and nulls can also be supplied as array keys, they get cast to strings or ints.

[colinodell]

True, but the challenge becomes differentiating between 0 vs 0.0 vs '0' vs 'false' in $recordsByLevel. Serializing the levels to strings (via serialize() or json_encode()) could help avoid this, but I think limiting valid (to us) keys to just strings and ints would probably be sufficient for virtually all sane use cases.

[jonathanjfshaw]

I wonder if maybe we should just change `string $level

If I'm understanding this right @colinodell, you've ended up suggesting that we don't need to relatively complicated level map approach proposed by @wimleers , and instead we can just replace all the string $level typehints with string|int $level.

As far as I can see the only drawback to this is that the magic hasDebugThatContains etc methods won't be usable by anyone who uses non-standard level parameters. That doesn't sound like a big problem to me, those methods are just a possible convenenience. And I'm not sure there is an easy solution for it - it's hard to do magic methods when literally anything could be the method name.

[jonathanjfshaw]

As far as I can see the only drawback to this is that the magic hasDebugThatContains etc methods won't be usable by anyone who uses non-standard level parameters. That doesn't sound like a big problem to me, those methods are just a possible convenenience. And I'm not sure there is an easy solution for it - it's hard to do magic methods when literally anything could be the method name.

Actually there is an easy solution.

I've opened a new PR #3 which
(1) Allows integer log levels on all methods, leaving it as the caller's responsibility to specify the correct level, there is no internal translation from/to LogLevel::* strings.
(2) Optionally allow a level map to be passed to the constructor, which has the sole purpose of allowing magic methods like hasDebugRecords().

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jonathanjfshaw]

Still relevant

…

On Mon, 6 Mar 2023, 23:41 stale[bot], ***@***.***> wrote: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. — Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2ZQQLP6UJZ7Q2VAKTSXZDW2ZY3RANCNFSM6AAAAAAS7VZ53I> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[colinodell]

v1.2.0 has been released with support for both string and int log levels!

Thanks for opening this issue and contributing the changes :D