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

[HttpKernel] Allow using #[WithLogLevel] for setting custom log level for exceptions #48747

Merged
merged 1 commit into from Jan 5, 2023

Conversation

angelov
Copy link
Contributor

@angelov angelov commented Dec 21, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR TODO

With these changes, we're extending the functionality for setting a custom log level for a given exception by introducing a new WithLogLevel attribute.

Example:

#[WithLogLevel(\Psr\Log\LogLevel::CRITICAL)]
class AccountBalanceIsNegative extends \Exception
{
}

This will have a lower priority compared to the log level set using the option in the configuration files.
(Meaning that if we have both an attribute declared on an exception class, and another level set for the same class in the configuration, the one from the configuration will be used.)

@carsonbot carsonbot added this to the 6.3 milestone Dec 21, 2022
@carsonbot carsonbot changed the title [HttpKernel][ErrorHandler] Allow using #[LogLevel] for setting custom log level for exceptions [ErrorHandler][HttpKernel] Allow using #[LogLevel] for setting custom log level for exceptions Dec 21, 2022
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks. Don't miss adding a changelog entry.

@angelov angelov changed the title [ErrorHandler][HttpKernel] Allow using #[LogLevel] for setting custom log level for exceptions [HttpKernel] Allow using #[LogLevel] for setting custom log level for exceptions Dec 21, 2022
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. Looks like a nice feature.

src/Symfony/Component/HttpKernel/Attribute/LogLevel.php Outdated Show resolved Hide resolved
final class LogLevel
{
/**
* @param \Psr\Log\LogLevel::* $level
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a use statement here to simplify the code?

Suggested change
* @param \Psr\Log\LogLevel::* $level
* @param LogLevel::* $level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot simplify it to just LogLevel because the attribute class has the same name, so we'll have to add an alias, thus I don't see any benefit from adding the use statement.

Copy link
Member

Choose a reason for hiding this comment

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

This naming conflict is indeed unfortunate because the one thing you want to pass to the constructor of the LogLevel attribute is a LogLevel constant. This makes the everyday use of the attribute a bit cumbersome. Maybe we should rename the attribute?

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'm open to suggestions as I couldn't think of a better name for the attribute.

Copy link
Member

@derrabus derrabus Dec 28, 2022

Choose a reason for hiding this comment

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

Dunno… AsLogLevel, WithLogLevel, AssignLogLevel, … 🤷🏻‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

MapLogLevel?
We already have a few Map* attributes
Worth renaming HttpStatus to MapHttpStatus?
Another idea could be At*

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about Map as it's used for argument mapping/binding/resolution AFAIK, and At feels weird for the status code. Another proposal: #[ToLogLevel] & #[ToHttpStatus]

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 renamed the class to WithLogLevel as it makes most sense to me. I can change it to something else if needed. I will also submit a PR to update the HttpStatus attribute accordingly after this is merged.

@angelov angelov force-pushed the log-level-attribute branch 2 times, most recently from 068b899 to 33c3a7c Compare December 29, 2022 18:24
@angelov angelov changed the title [HttpKernel] Allow using #[LogLevel] for setting custom log level for exceptions [HttpKernel] Allow using #[WithLogLevel] for setting custom log level for exceptions Dec 29, 2022
@fabpot
Copy link
Member

fabpot commented Jan 5, 2023

Thank you @angelov.

@fabpot
Copy link
Member

fabpot commented Jan 5, 2023

As a follow-up, I've created #48876 to rename HttpStatus to WithHttpStatus.

@angelov angelov deleted the log-level-attribute branch January 5, 2023 08:37
fabpot added a commit that referenced this pull request Jan 6, 2023
…tus (fabpot)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpKernel] Rename HttpStatus atribute to WithHttpStatus

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes-ish
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Refs #48352 and #48747

As discussed in the 2 referenced PRs for better naming consistency.

Commits
-------

fece766 [HttpKernel] Rename HttpStatus atribute to WithHttpStatus
@fabpot fabpot mentioned this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants