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.
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.
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.
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.
What is the sense behind this block? I don't understand what is tested here.
There was a problem hiding this comment.
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.
@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.
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\LevelandRollbar\Payload\Level.#140