-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Allow #[WithHttpStatus]
and #[WithLogLevel]
to take effect on interfaces
#53191
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
Conversation
#[WithHttpStatus]
to take effect on interfaces
How would that work with the exception mapping already handled ? Would there not be conflicts / priority troubles there ? |
Parent classes are prioritized first, starting from the immediate parent first. Then, interfaces are processed, according to the order of |
If i was not precise enough, i was making allusion to the framework exception map that already exists and... map classes or interfaces to HTTP status / Log level : https://symfony.com/doc/current/reference/configuration/framework.html#exceptions I just find a bit odd that those two implementations both coexists in the ErrorListener (one before and one after) foreach ($this->exceptionsMapping as $class => $config) {
if (!$throwable instanceof $class || !$config['status_code']) {
continue;
}
if (!$throwable instanceof HttpExceptionInterface || $throwable->getStatusCode() !== $config['status_code']) {
$headers = $throwable instanceof HttpExceptionInterface ? $throwable->getHeaders() : [];
$throwable = new HttpException($config['status_code'], $throwable->getMessage(), $throwable, $headers);
$event->setThrowable($throwable);
}
break;
} |
Yes, doing it from the configuration is the traditional way to do it before 6.3, and it is still working as intended. https://symfony.com/blog/new-in-symfony-6-3-http-exception-attributes |
Looking at the implementation, I think this might be too unusual - ie unexpected. The relationship is unnatural to me. Why don't you leverage |
That would require my domain model to depend on HttpKernel. Without this change, I could still do it using |
#[WithHttpStatus]
to take effect on interfaces#[WithHttpStatus]
and #[WithLogLevel]
to take effect on interfaces
Added more background information in the PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, LGTM!
…effect on interfaces
Thank you @priyadi. |
This change lets adding
#[WithHttpStatus]
and#[WithLogLevel]
to an interface, and exceptions implementing such interface will behave as if#[WithHttpStatus]
and#[WithLogLevel]
is attached to itself.#[WithHttpStatus]
and#[WithLogLevel]
were created to have configuration likeconfig/packages/exceptions.yaml
but using attributes: https://symfony.com/blog/new-in-symfony-6-3-http-exception-attributesHowever, unlike
exceptions.yaml
,#[WithHttpStatus]
and#[WithLogLevel]
did not previously work with interfaces. Thus, the two ways of configuring have slightly different semantics. This PR fixes the issue.