Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
#[WithHttpStatus] to take effect on interfacessmnandre commentedDec 23, 2023
How would that work with the exception mapping already handled ? Would there not be conflicts / priority troubles there ? |
priyadi commentedDec 23, 2023
Parent classes are prioritized first, starting from the immediate parent first. Then, interfaces are processed, according to the order of |
smnandre commentedDec 23, 2023
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 theErrorListener (one before and one after) foreach ($this->exceptionsMappingas$class =>$config) {if (!$throwableinstanceof$class || !$config['status_code']) {continue; }if (!$throwableinstanceof HttpExceptionInterface ||$throwable->getStatusCode() !==$config['status_code']) {$headers =$throwableinstanceof HttpExceptionInterface ?$throwable->getHeaders() : [];$throwable =newHttpException($config['status_code'],$throwable->getMessage(),$throwable,$headers);$event->setThrowable($throwable); }break;} |
priyadi commentedDec 23, 2023
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 |
nicolas-grekas commentedDec 23, 2023
Looking at the implementation, I think this might be too unusual - ie unexpected. The relationship is unnatural to me. Why don't you leverage |
priyadi commentedDec 23, 2023
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 interfacespriyadi commentedDec 24, 2023
Added more background information in the PR description. |
nicolas-grekas left a comment
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
nicolas-grekas commentedDec 27, 2023
Thank you@priyadi. |
Uh oh!
There was an error while loading.Please reload this page.
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.yamlbut 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.