Allow mixed types for $level and normalize them in case non-int|string levels are passed in #24
Conversation
|
cc @Bilge @derrabus @MidnightDesign @sasezaki any thoughts here? |
src/Test/TestLogger.php
Outdated
| /** | ||
| * @param mixed $level | ||
| */ | ||
| private function normalizeLevel($level): int|string |
There was a problem hiding this comment.
Marked static and protected as well because #10 shows this might come in handy
src/Test/TestLogger.php
Outdated
| $message = $this->interpolate($message, $context); | ||
| } | ||
|
|
||
| $level = $this->normalizeLevel($level); |
There was a problem hiding this comment.
That's a breaking change though..?
There was a problem hiding this comment.
Oh, I get it. The $recordsByLevel index could never have worked with anything else.
There was a problem hiding this comment.
Right, I don't believe this carries much risk as it was broken already per #10
src/Test/TestLogger.php
Outdated
| * | ||
| * @psalm-type log_record_array array{level: string, message: string|\Stringable, context: mixed[]} | ||
| * @psalm-type has_record_array array{level?: string, message?: string|\Stringable, context?: array<array-key, mixed>} | ||
| * @psalm-type log_record_array array{level: mixed, message: string|\Stringable, context: mixed[]} |
There was a problem hiding this comment.
If you're normalizing it to string|int, this can also be string|int, right?
There was a problem hiding this comment.
No, normalization means exactly "input may be everything (mixed) and output is string|int".
There was a problem hiding this comment.
Maybe I'm not understanding correctly, but what I'm saying is: the log_record_array is only used on $records and $recordsByLevel - and both of them can never have anything other than string|int in the level offset. At least not after this PR.
There was a problem hiding this comment.
Yes for this type @MidnightDesign is correct IMO, I have changed it.
There was a problem hiding this comment.
The $level of the query methods can be kept a bit narrower: string|int|Stringable|UnitEnum. They can't respond with true to anything else.
|
Seems fine. |
src/Test/TestLogger.php
Outdated
| /** | ||
| * @param mixed $level | ||
| */ | ||
| protected static function normalizeLevel($level): int|string |
There was a problem hiding this comment.
I get that exposing this to extending classes is on purpose, so that they can implement custom normalization behavior, but is that really worth increasing the API surface?
Also, if this behavior should be allowed to be modified, wouldn't composition (optionally passing in some kind of normalizer) be better than the inheritance route?
There was a problem hiding this comment.
composition for a class that has so many public methods is a pain IMO if all you want to do is add normalization for a custom logger..
But maybe we're better off leaving this private until a real use case materializes, because enum+stringable support should already cover most use cases out there.
There was a problem hiding this comment.
You're still calling normalizeLevel() with $this-> in multiple places.
|
Alright then, thanks everyone. I'll wrap up a release to get all this out. |
| /** | ||
| * @param mixed $level | ||
| */ | ||
| private static function normalizeLevel($level): int|string |
There was a problem hiding this comment.
| /** | |
| * @param mixed $level | |
| */ | |
| private static function normalizeLevel($level): int|string | |
| private static function normalizeLevel(mixed $level): int|string |
?
There was a problem hiding this comment.
Right, I thought this was from php8.1 somehow.. Not that huge a deal anyway
Fixes #10
Fixes #22