Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(PsrLoggerAdapter): Allow to use Psr\Log\LogLevel for log method #47293

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 16, 2024

Summary

There is the Psr\Log\LogLevel class defining log level constants, to be fully compatible we should at least support those logging levels. Moreover this is the last part that was still required from ILogger interface, as we did not have alternatives for the log level constants.

Checklist

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I’m not sure I get the point, those const are strings and not int, so we will still not be able to use them instead of the ILogger ones?

The problem is that there is no standard for int levels.
We should host those const elsewhere or undeprecate them in my opinion. But core and apps need a way to reference log levels by name.

Should we add a backed enum in OCP for that 🤓 ?

@susnux
Copy link
Contributor Author

susnux commented Aug 19, 2024

I’m not sure I get the point, those const are strings and not int, so we will still not be able to use them instead of the ILogger ones?

Well this adds support so that you can call:

LoggerInterface $logger;

// ...

$logger->log(LogLevel::WARNING, 'this is a warning')
$logger->log(LogLevel::DEBUG, 'this is a debug message')

So everywhere¹² you use the ILogger levels you can now also use the PSR levels.


¹ Well not if directly using the Log class but that is private and we should then also remove ILogger from public scope.
² And of course level set in the context would be a "magic" number, this is a problem if you use the "new log message"-event.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Thanks! This is great when passing our logger instance into a library that speaks to psr/log but obviously has no idea about our constants

@come-nc
Copy link
Contributor

come-nc commented Aug 19, 2024

There is less usage of those consts than I thought. I was expecting to find a lot of >= comparison and there is none 🤔
Still, the config entry loglevel will stay an int, right?

@susnux susnux force-pushed the feat/logger-allow-psr-loglevel branch 2 times, most recently from 34accb6 to 02e9c30 Compare September 14, 2024 15:34
There is the `Psr\Log\LogLevel` class defining loglevel constants,
to be fully compatible we should at least support those logging levels.
Moreover this is the last part that was still required from `ILogger` interface,
as we did not have alternatives for the loglevel constants.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the feat/logger-allow-psr-loglevel branch from 02e9c30 to 81df2d2 Compare September 14, 2024 15:35
@susnux susnux merged commit 9bfbbd9 into master Sep 14, 2024
174 checks passed
@susnux susnux deleted the feat/logger-allow-psr-loglevel branch September 14, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants