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

Add listen new config param "minimum_level" error log. #44

Closed

Conversation

Yozhef
Copy link

@Yozhef Yozhef commented Mar 15, 2019

Hello everybody.

Problem:
In the project, I am currently working on it. There is a createRollbarHandler function in the RollbarHandlerFactory class. This LogLevel parameter accepts only Error / Critical values. It would be great if it is possible to put it. Because info info includes Warning / Warning / Error / Critical.

Solution for the problem:
I created you PR on rollbar-php when i add new param in config minimum_level, and this i use him and determine which error level is required.

Example config:

rollbar:
    minimum_level: 'warning'

}

$this->minimumLevel = $config['minimum_level'] ?: LogLevel::ERROR;

Choose a reason for hiding this comment

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

Same here, I recommend using monolog_minimum_level for the config option name as suggested in rollbar/rollbar-php#440 (review)

Also, I think isset($config['minimum_level']) is needed here as I get PHP notices if minimum_level is not set in rollbar.yaml:

$this->minimumLevel = isset($config['minimum_level']) ? $config['minimum_level'] : LogLevel::ERROR;

Copy link
Author

@Yozhef Yozhef Mar 18, 2019

Choose a reason for hiding this comment

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

@ArturMoczulski okay, i change it validation, but I do not like the name of this option. I answer rollbar/rollbar-php#440 (review)

@Yozhef Yozhef force-pushed the listenNewConfigMinimumLevelError branch from 9ef0e05 to 1cec8e1 Compare March 18, 2019 07:19
@Yozhef
Copy link
Author

Yozhef commented Mar 18, 2019

@ArturMoczulski I added as you requested, but I think verification is not required because we add minimum_level -> default valuer LogLevel::ERROR.

I think if we add to (default, config) value need on code only get.

  1. If a person does not use in his config yaml this params -> аutomatically uses value from default.
  2. If a person use on his config this param, he override default values.

@Yozhef Yozhef force-pushed the listenNewConfigMinimumLevelError branch from 1cec8e1 to 6aed6c7 Compare March 18, 2019 07:46
@ArturMoczulski
Copy link

Let's continue this work in #46

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

Successfully merging this pull request may close these issues.

2 participants