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

Resolve "level" env var at runtime #446

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Nov 26, 2022

Resolves #413, #444

Currently, the log level is read when the container built. This means we cannot change a log level at runtime using env vars.

With this change, log level can be modified for a specific command run: LOG_LEVEL=debug bin/console ...

I would like to get some feedbacks before updating tests. I don't know yet what is the best strategy to check the resolution of the values.

@GromNaN GromNaN changed the title Resolve level env var at runtime Resolve "level" env var at runtime Nov 28, 2022
MonologBundle.php Outdated Show resolved Hide resolved
@uncaught
Copy link

uncaught commented Dec 2, 2022

Did you check if this is even needed?

Last time I checked, every monolog handler already casts the log level.

I see no reason for this bundle to cast the level at all.

@GromNaN GromNaN requested a review from lyrixx December 2, 2022 22:01
@GromNaN
Copy link
Member Author

GromNaN commented Dec 2, 2022

Why didn't I think of that! You are absolutely right.

By default the AbstractHandler constructor calls setLevel, which calls Logger::toMonologLevel($level).

I'll check for older versions and update the PR.

@GromNaN
Copy link
Member Author

GromNaN commented Dec 2, 2022

Logger::toMonologLevel is called since monolog 1.11. This is safe to remove the call in the DI extension.

@GromNaN GromNaN force-pushed the level-factory-enum branch 3 times, most recently from b6e7432 to 6204234 Compare December 2, 2022 23:04
@GromNaN GromNaN requested a review from Seldaek December 2, 2022 23:14
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

I did not test the code locally, but this seems super cool 👍🏼
Less code => more feature 🎖️

Copy link
Member

@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

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

Also seems good to me just having a look at the code 👍🏻

@lcobucci
Copy link

@GromNaN do you have an idea on when this will be released?

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.

Setting log_level via environment variable requires cache clear
5 participants