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

fix: getLogLevel return type #44258

Merged
merged 1 commit into from Mar 17, 2024
Merged

fix: getLogLevel return type #44258

merged 1 commit into from Mar 17, 2024

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Mar 17, 2024

Fixes #43534

Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv skjnldsv requested review from ArtificialOwl, susnux, SystemKeeper, a team, Fenn-CS and sorbaugh and removed request for a team March 17, 2024 13:10
@skjnldsv skjnldsv self-assigned this Mar 17, 2024
@skjnldsv skjnldsv added 3. to review Waiting for reviews regression labels Mar 17, 2024
@skjnldsv skjnldsv requested a review from Altahrim March 17, 2024 13:10
@susnux
Copy link
Contributor

susnux commented Mar 17, 2024

Ah yes had the same issue as something set the default to '0'. But maybe we should add some logging for this case to warn the user that the log level config is invalid?
Maybe something like:

try {
   $loglevel = (int)$this->config->getValue('loglevel', ILogger::WARN);
} catch (\TypeError $e) {
  // can we use the logger here? if not then use errorlog
}

The reason would be to prevent confusion why the loglevel is not respected, got some similar issues in the past leading to wasted hours of debugging 😅

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Works but I think the logging (see commit) would make sense

@susnux susnux merged commit 575fb8d into master Mar 17, 2024
167 checks passed
@susnux susnux deleted the fix/log_level branch March 17, 2024 18:58
Comment on lines +284 to +285
$configLogLevel = $this->config->getValue('loglevel', ILogger::WARN);
return min(is_int($configLogLevel) ? $configLogLevel : ILogger::WARN, ILogger::FATAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

As @susnux pointed out we should log an error or cast the value to int.

Copy link
Member

Choose a reason for hiding this comment

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

Ref #44262

@Altahrim Altahrim mentioned this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants