-
Notifications
You must be signed in to change notification settings - Fork 121
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
GitHub Issue #140: Refactor usage of levels in log methods #199
Conversation
src/Payload/Level.php
Outdated
@@ -2,6 +2,17 @@ | |||
|
|||
class Level implements \JsonSerializable | |||
{ | |||
const IGNORE = 'ignore'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these two "ignore(d)" levels for? They don't map to any level we have on the backend. Same for "alert" and "emergency".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IGNORE
and IGNORED
are synonymous and they can be used to completely silence the logger if used in minimumLevel
config option. I think it's mostly a leftover from 0.18.2, I don't really see the point and it's also missing from README.md. I highly doubt anybody is using them so I'm happy to remove them.
ALERT
and EMERGENCY
are PSR-3 levels and are mapped to Rollbar levels here: https://github.com/rollbar/rollbar-php/blob/master/src/Payload/Level.php#L16-L33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! Thanks for the info. WOuld you mind adding a comment to this code and point to where they are mapped to the Rollbar levels?
Much cleaner! |
…evel object creation to the DataBuilder; remove usage of static calls to Level and replace them with a LevelFactory
…e Config responsible for providing LevelFactory dependency to the DataBuilder
This is ready for review @coryvirok @rokob |
tests/LevelTest.php
Outdated
$level = Level::TEST(); | ||
$this->fail(); | ||
} catch (\Exception $exception) { | ||
$this->assertTrue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the sense behind this block? I don't understand what is tested here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be testing that Level::TEST()
throws an exception. The alternative would be to use expectException()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olafnorge this tests that an exception is thrown if a static method is called on Level
that does not match any of the logging levels. Mostly this doesn't matter, as static methods are now deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, thanks for clarifying.
This looks good to me, we will need to have a bump in version beyond the bugfix level because this changes the API |
Wait I am wrong about that, this uses the |
@rokob Yes, this is backward compatible. The static methods still work but I marked that as deprecated in favor of |
This removes most of the usage of static calls to Level class and clarifies the confusion between using
Psr\Log\Level
andRollbar\Payload\Level
.#140