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 to config minimumLevel option. #440

Closed
wants to merge 3 commits into from
Closed

Conversation

Yozhef
Copy link
Contributor

@Yozhef Yozhef commented Mar 13, 2019

Hello everybody.

Please add the configuration property of the minimum error level.

My problem is:
I have 2 projects in one, I have to log errors from Error and in another from Info. Therefore, I need the option to configure the level of the class Config -> $options -> in my yml required error level.

@brianr
Copy link
Member

brianr commented Mar 14, 2019

Thanks @Yozhef!

@ArturMoczulski is there a way to do this already? If not, this seems like a reasonable request to me.

@ArturMoczulski
Copy link
Contributor

@Yozhef thanks for submitting the PR

However, I actually can't figure out what is the feature you need here. The PR only contain adding the minimumLevel config option, but it is not actually used.

From what you described though, it seems you want to have conditional error levels depending on the environment or the config file loaded. Did I get that right? I'm not sure how a feature called minimumLevel would facilitate that.

Can you guide here what you are actually after?

@Yozhef
Copy link
Contributor Author

Yozhef commented Mar 15, 2019

@ArturMoczulski I need this parameter on other project Rollbar company rollbar-php-symfony-bundle. On this project error level is harcode on createRollbarHandler function in RollbarHandlerFactory class. I need configure minimum error level on my yaml config. I create PR on use new config =)

P.S Would be grateful if you made a revue here here too

@ArturMoczulski
Copy link
Contributor

Ok. Now I see. Thanks for the explanation. I'll leave some feedback on the PR

src/Config.php Outdated
@@ -56,7 +56,8 @@ class Config
'include_raw_request_body',
'local_vars_dump',
'max_nesting_depth',
'max_items'
'max_items',
'minimum_level'
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this option would only be used with the Monolog handler. I suggest changing this config option to monolog_minimum_level. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is better because this params is found and maybe people use this. You have a good test how it's work. And very long name monolog_minimum_level.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is with the possible confusion of what this config option might be. This is something that will be only used with Monolog handlers. If we don't make that crystal clear a lot of users might mistakenly assume the config option will apply even if they don't use Monolog. Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ArturMoczulski I thought and still reworked on your offer. =)

src/Defaults.php Outdated
@@ -358,4 +363,5 @@ public function maxItems($value = null)
private $scrubFields;
private $customTruncation = null;
private $maxItems = 10;
private $minimumLevel = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We also should probably default this to \Monolog\Logger::error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's good =)

@ArturMoczulski
Copy link
Contributor

Thanks for those changes @Yozhef . Actually, I made a mistake. As you could see we already had the minimumLevel property, it just got deprecated as a config option. We just need to bring it back and then use it in the Symfony bundle.

I continue the work on this in #442 and will be merging in a moment. Let's continue our conversation there.

@ArturMoczulski
Copy link
Contributor

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.

3 participants