Narrow supported log level phpdoc

[mvorisek]

Only the LogLevel::* log levels need to be supported per https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-3-logger-interface.md#11-basics

User can still widen the accepted type like LogLevel::*|'xxx' thanks to LSP.

[Seldaek]

I guess this makes sense yes.

[mvorisek]

Can it be merged and released?

[Jean85]

This may require a change in the spec..

[mvorisek]

This may require a change in the spec..

@Jean85 please see the linked spec in the PR description, citing:

A ninth method, log, accepts a log level as the first argument. Calling this method with one of the log level constants MUST have the same result as calling the level-specific method. Calling this method with a level not defined by this specification MUST throw a Psr\Log\InvalidArgumentException if the implementation does not know about the level. Users SHOULD NOT use a custom level without knowing for sure the current implementation supports it.

So I belive the spec. allows this change, as the phpdoc in this PR is basically equivalent to the "Calling this method with a level not defined by this specification MUST throw".

[mvorisek]

I belive I have prooven what needs to be proven, can this PR be merged, please 🙏?

[KorvinSzanto]

This is a breaking change and disallows the valid use case of an implementor adding a custom log level: https://www.php-fig.org/psr/psr-3/#11-basics

This could only merge if mixed is still allowed.

Edit: To add some more context now that I'm not on mobile, the current method accepts mixed. While implementors can widen the type if they choose to, consumers cannot and so by changing this type we are disallowing the following:

function logSomething(LoggerInterface $logger): void
{
    try {
        $logger->log(new MyCustomLogLevel(), 'foo');
    } catch (\InvalidArgumentException $e) {
        $logger->log('warning', 'foo'); // Fallback to supported type
    }
}

To me this is a breaking change even if the error only shows up in static analysis. It seems easy and obvious to simply change the type to mixed|LogLevel::* in order to keep backwards compatibility and gain the IDE sugar you're looking for.

[KorvinSzanto]

See my comment. To demonstrate my concern, compare these:

• Current mixed type: https://psalm.dev/r/b15a7f5725
• PR proposed type: https://psalm.dev/r/8e53dc846d
• My recommendation: https://psalm.dev/r/e4306f884a

[mvorisek]

@KorvinSzanto please see #79 (comment) - the spec already defines:

Users SHOULD NOT use a custom level without knowing for sure the current implementation supports it.

that the default interface does not support any other log level by default.

Thank to LSP, user can widen the accepted type:

• https://phpstan.org/r/874bb24d-4daa-479c-80ba-08f5ebb07e65
• https://psalm.dev/r/3c98113bc9

working in both/all static analysers.

Given these facts, I belive this PR is not violating the spec.

[KorvinSzanto]

@KorvinSzanto please see #79 (comment) - the spec already defines:

Users SHOULD NOT use a custom level without knowing for sure the current implementation supports it.

that the default interface does not support any other log level by default.

SHOULD NOT means that something is allowed but discouraged https://datatracker.ietf.org/doc/html/rfc2119#section-4 :

4. SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that
there may exist valid reasons in particular circumstances when the
particular behavior is acceptable or even useful, but the full
implications should be understood and the case carefully weighed
before implementing any behavior described with this label.

And so for the purposes of this discussion, in my opinion, the quote you show is effectively the same as "Users MAY use a custom level without knowing ..." and by changing the type here to disallow that use case we're going from SHOULD NOT (or MAY) to MUST NOT since you must know and use a subtype for this functionality to work.

Thank to LSP, user can widen the accepted type:

• https://phpstan.org/r/874bb24d-4daa-479c-80ba-08f5ebb07e65
• https://psalm.dev/r/3c98113bc9

working in both/all static analysers.

That's not an argument for narrowing the type here though. Most implementors simply inherit the type from the parent, for example symfony:
https://github.com/symfony/symfony/blob/7.0/src/Symfony/Component/HttpKernel/Log/Logger.php#L93 and so what you are advocating for is implementors change their code to maintain compatibility with the current API which is, to me, a breaking change.

Given these facts, I belive this PR is not violating the spec.

I disagree. Can you take a second and speak to the value of narrowing this type rather than just using an intersection or union type with mixed?