Update parameter and property types for version 2

[Crell]

As per the upgrade bylaw.

• Removes the long-outdated test classes. (This mandates a 2.0 release at least.)
• Add parameter types.
• Add one property type.
• The spec very explicitly allows string|Stringable, so this requires PHP 8.0.
• I also folded AbstractLogger and LoggerTrait together, since they're identical and there's no reason to support PHP 5.3 anymore. I am not certain if this should be considered kosher since AbstractLogger is specified in the spec. It's in its own commit so easy to undo if we decide to.

[shadowhand]

At what point can we get rid of the redundant @param and @return docs? Having both types and doc'd types means there is a higher chance that the two will get out of sync and cause confusion.

[shadowhand]

Can we remove docblock types as part of this PR?

[Crell]

As the docblock types are directly in the spec, I'd say no. We didn't on the other type updates for other specs, IIRC. I totally get the argument that they're redundant at this point, but it's not like the file is going to change often enough for them to get out of sync.

[Jean85]

I agree with @Crell for the PHPDoc. Also, we could enforce correctness by introducing a static check with Psalm or PHPStan if we want, now!

Since we're ditching the test classes, do we want to split them into an log-util repo? Do you want me to create it?

[Crell]

If someone wants those test classes to still exist, a log-util repo would be the place for them. I personally have no interest in them so won't maintain it, so let's see if anyone else wants to volunteer. 😄

[Crell]

Inquiry. As long as we're doing this, should we also restructure the package to use PSR-4 instead of rooted PSR-0? I'm leaning toward yes.

[Seldaek]

Looks good to me, thanks @Crell for getting it done. However one question remains for me: What's the point of having a 2.0 and 3.0 if both anyway require PHP 8? Does it really provide a better upgrade path for libraries? I guess only implementers already requiring PHP 8 would benefit, which is probably not a lot, if any.

Anyway I'm fine still merging as is to keep it by the book.

[Seldaek]

As for test classes yes it would be good to keep them IMO so implementers can verify they adhere to the spec. @Jean85 please create it, I'd say it's not so urgent as I doubt anyone can upgrade to v2 here any time soon due to the PHP 8 req, but if nobody else does it I'll eventually get the test classes back in that util lib so I can upgrade monolog to it.

[stof]

• I am not certain if this should be considered kosher since AbstractLogger is specified in the spec.

that's fine to me. The behavior of AbstractLogger is still the same, even if it uses the trait to implement it internally. The usage of the trait is an implementation detail here.

Looks good to me, thanks @Crell for getting it done. However one question remains for me: What's the point of having a 2.0 and 3.0 if both anyway require PHP 8? Does it really provide a better upgrade path for libraries? I guess only implementers already requiring PHP 8 would benefit, which is probably not a lot, if any.

It allows implementers to support ^1.0 || ^2.0 or ^2.0 || ^3.0, to increase support progressively. If the implementation classes are not final, supporting the 3.0 signature requires a major version bump for the implementation (see our work in symfony/cache for the support of new psr/cache versions, where implementing psr/cache 3 has to wait until Symfony 6.0 due to that).
It reduces the risk of version conflicts in composer.

[Jean85]

Inquiry. As long as we're doing this, should we also restructure the package to use PSR-4 instead of rooted PSR-0? I'm leaning toward yes.

Yeah we should IMHO. Since we're requiring such a newer PHP version, requiring PSR-4 should be so harsh on end users.

[Crell]

Switched to an src directory. Someone please check that I got it all right; I'd love for us to release a new package and NOT have to rush out a 2.0.1 release right after because we broke Symfony somehow... 😉

[Jean85]

If someone wants those test classes to still exist, a log-util repo would be the place for them. I personally have no interest in them so won't maintain it, so let's see if anyone else wants to volunteer. smile

Repo created: https://github.com/php-fig/log-util
@php-fig/psr-3 team added as mantainer, and @Crell you're added to the team.

@XedinUnknown you expressed interest with #75, are you willing to submit a PR there porting the classes? I'm up for reviewing and improving that if you want.

[XedinUnknown]

@Jean85, absolutely. I'll let you know shortly, probably going to work on this over the weekend.

One thing, though. I don't actually have PHP installed on my host machine, because I work with Docker, and this is where all dev things reside - like in https://github.com/Dhii/php-project. I also add tests for interfaces to make sure that implementations can actually be instantiated. Would you accept also a Docker setup, static analysis tools, maybe a PHPStorm config as well in the repo? .gitattributes will be properly configured to exclude that stuff from dist. If no - no problem, I can just .gitignore it.

[Jean85]

@XedinUnknown no I don't think it would belong there. I was thinking about putting up Github actions as CI (separate PR maybe?), but other stuff would be too specific IMHO.

[XedinUnknown]

Ok. I also use GH Actions.

What would it do though, if there aren't any tools?

[Jean85]

What would it do though, if there aren't any tools?
Well including PHPUnit test cases means requiring (in dev) PHPUnit. It should run those tests against a plain implementation to check that everything works, in regards to all the PHPUnit versions declared as compatible.

This way, we can version the package against the moving target of PHPUnit versions.

[XedinUnknown]

Ok, so I guess it's mostly about proving that deps are installable, than proving that the code works, in this case. I kinda do something similar in #75. 👍

[XedinUnknown]

If I may suggest something, since this is going to be a BC-breaking release anyway. At the same time, it could require a 3.0 release (less steep upgrade path), so it's more about how I see an abstract perfect future.

A standard is a standard, and IMO it should exist separately from any implemenatation, since implementation is about the "how", and a standard is not. Therefore, from that perspective, perhaps it could make sense to:

• Switch the @throws type from Psr\Log\InvalidArgumentException to \InvalidArgumentExceptin (not sure what value this coupling to a package-specific class adds), or maybe to a new Psr\Log\InvalidArgumentExceptionInterface. Or, even better, see other point about exceptions below.
• Switch from Psr\Log\LogLevel to a Psr\Log\LogLevelInterface. At first I thought it would be better if the log level constants can simply be part of the logger itself, but I can see this separate class as a step towards upcoming enums, and that's very cool. But again, a class is an implementation.
• Move all remaining implementation to php-fig/log-util. I understand the value of an abstract and no-op implementation. Still, it's implementation detail.
• Right now, log() is only allowed to throw the InvalidArgumentException, which is arguably not very friendly, as it doesn't explain at all in which cases the exception would be thrown (e.g. what it means). And if it means that the log level is invalid (which is arguably more of an e.g. RangeException, because InvalidArgumentException is explicitly for invalid types, and not for invalid values), then this does not allow throwing in case something actually goes wrong during logging - like a stream or DB or HTTP error, etc. So perhaps adding a LoggerExceptionInterface similar to ContainerExceptionInterface could be beneficial. Unlike ContainerExceptionInterface, however, I would love it if this exception actually exposed some detail about what has happened, like LoggerExceptionInterface::getLogger(): LoggerInterface. This could be the generic exception, and a CouldNotLogExceptionInterface extends LoggerExceptionInterface could expose a ::getLogLevel(), ::getLogMessage(), and ::getLogContext().

I absolutely do not mean to annoy anyone by the above, and if I didn't manage to avoid that, then I take it back, and please let's pretend I never wrote it 😅.

[Crell]

Independent of the value of any of the suggestions above, I think most of them would be breaking changes requiring a new PSR anyway. So these responses are largely academic.

1. The marker exception is there so that any exceptions thrown are easily identifiable as coming from a Log package. In hindsight an interface would have been better, but we hadn't learned our lesson at that point.
2. Again, this would probably have been wise but would be a breaking change. I'm working on a class in TYPO3 right this minute that extends LogLevel, for better or worse, so changing it would break TYPO3.
3. Once again, were this spec made today I would agree. But moving code out of the package into a new util package is a breaking change, so we can't do that.
4. It can throw that exception or children thereof. Implementers are welcome and encouraged to subclass it further to be more precise in their thrown errors.

In general, yeah, not bad ideas, but they'd all require a new PSR given the level of change they are. The whole point of the type additions is that, thanks to variance rules, the two-step process allows us to not actually have any hard breaks. There's always an overlap that allows people to upgrade gradually.

[stof]

• Switch from Psr\Log\LogLevel to a Psr\Log\LogLevelInterface. At first I thought it would be better if the log level constants can simply be part of the logger itself, but I can see this separate class as a step towards upcoming enums, and that's very cool. But again, a class is an implementation.

Switching to an interface does not make sense IMO. This LogLevel class is semantically an enum (but requiring PHP 8.1 to make it an actual enum is too bleeding edge for now)

[XedinUnknown]

@stof, I agree with that. My only problem is that classes are implementation, while enums are actual contracts (even if not formally an interface perhaps).

[XedinUnknown]

@Crell, thanks for the detailed response, much appreciated!

It can throw that exception or children thereof. Implementers are welcome and encouraged to subclass it further to be more precise in their thrown errors.

The thing is, nothing right now seems to declare Psr\Log\InvalidArgumentException as the root exception of the logger: there are no docs for the @throws that would explain in which circumstances that exception can be thrown, and therefore the semantic is IMO derived from the type, which is an \InvalidArgumentException, and that has very specific meaning - which is not related to logging at all.

If such breaking changes are not possible (although I thought they are, as long as there is a gradual upgrade path), then perhaps changing the @throws to something like this would suffice:

@throws Psr\Log\InvalidArgumentException If problem logging.

This way, implementors and consumers should know that any subtype of Psr\Log\InvalidArgumentException indicates a problem with logging, not necessarily with an invalid argument to the log() method, and its subtypes can represent more specific causes for the failure, while still being semantically a logging failure.

In addition to that, no other method except log() appears to be allowed to throw any exception, let alone the specific Psr\Log\InvalidArgumentException of which the consumers of e.g. error() simply cannot know - without depending on a concrete implementation.

That dependency on concretion is furthered by any consumers of LoggerInterface who depend on speific subtypes of Psr\Log\InvalidArgumentException that are not declared by the LoggerInterface, by the way, since there's no way to know about them when consuming the interface.

[stof]

@XedinUnknown if psr/log was created in the PHP 8.1+ era, LogLevel would be an enum. But it must live with its requirements, and so use an implementation that works on existing PHP versions. This means using a class with constants.
LogLevel should probably be tagged as @final to discourage extending it though (making it actually final is a BC break that would affect some projects like TYPO3, so too harsh without a prior warning IMO).

[XedinUnknown]

@stof, yes, of course I understand that. My suggestion was to make it an interface, with the same class constants, etc.

Regarding final, though: why would it be a good idea to limit how developers can extend and tailor for their own needs, if they want to add some other domain-specific log levels? 🤔

[lyrixx]

TestLogger class was quite usefull. Why did you remove it ?

[Jean85]

We split that away into https://github.com/php-fig/log-util so it would make it easier to handle PHPUnit upgrades and the like.
We're probably a bit stuck there, maybe we should push that forward!

[lyrixx]

But there is not relation to phpunit at all. I don't understand

[Jean85]

I'm sorry, I made a mistake and read it wrongly, I was thinking about the TestCase.

@Crell any reason?

[Crell]

The psr package should include those symbols defined in the PSR; no more, no less. Putting anything else in this package in the first place was a mistake. (Honestly the traits and abstract classes here are also a mistake and should have been in a util package rather than the spec itself, but this was before we did util packages.) Anything not listed in the spec itself belongs in a util package, regardless of whether it relates to testing.

I fully agree that a mock logger for testing is something the util package should have.