Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Extend compatibility with newer version of psr/log:"^1.1.2|^2.0|^3.0" #50

Closed
wants to merge 3 commits into from

Conversation

10n
Copy link

@10n 10n commented Jun 9, 2023

Q A
New Feature yes

Description

Extend compatibility with newer version of psr/log:"^1.1.2|^2.0|^3.0"

  • updated \LaminasTest\Log\PsrLoggerAdapterTest, removed dependency on \Psr\Log\Test\LoggerInterfaceTest (it was removed in newer versions).

froschdesign and others added 2 commits June 9, 2023 20:05
Signed-off-by: Frank Brückner <[email protected]>
Signed-off-by: Ionut Cioflan <[email protected]>
- Remove dependency to \Psr\Log\Test\LoggerInterfaceTest

Signed-off-by: Ionut Cioflan <[email protected]>
String "Laminas\Log\Writer\Firephp" contains class reference, use ::class instead

Signed-off-by: Ionut Cioflan <[email protected]>
* @throws InvalidArgumentException If log level is not recognized.
*/
public function log($level, $message, array $context = [])
public function log($level, $message, array $context = []): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please not add type information where it would break subclasses

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this because this was enforced by in psr/log 3.0
https://github.com/php-fig/log/blob/3.0.0/src/LoggerTrait.php

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I was thinking that this was just a wrapper to offer compatibility from laminas/log to psr/log ... most likely it was used as is, but you are right, it can break sub classes..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to put additional effort into this (make different branch for targeting different psr/log versions)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this component should really not be touched: moving to Monolog\Monolog is endorsed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this component should really not be touched: moving to Monolog\Monolog is endorsed.

Understoo, thank you.

@Ocramius
Copy link
Member

Ocramius commented Jun 9, 2023

Hmm, this component is security-only, and shouldn't receive new updates 🤔

@10n
Copy link
Author

10n commented Jun 9, 2023

Hmm, this component is security-only, and shouldn't receive new updates 🤔

I've tried to make it compatible. Because it targets psr/log: 1.* it may be conflicting with other libraries that may choose to use newer versions. I can accept that it's security-only ... and use the fork as a replacement package.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants