Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
nicolas-grekas merged 1 commit intosymfony:7.1frompriyadi:interface-withhttpstatus
Dec 27, 2023
Merged

[HttpKernel] Allow#[WithHttpStatus] and#[WithLogLevel] to take effect on interfaces#53191

nicolas-grekas merged 1 commit intosymfony:7.1frompriyadi:interface-withhttpstatus
Dec 27, 2023

Conversation

@priyadi
Copy link
Contributor

@priyadipriyadi commentedDec 22, 2023
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
Issues
LicenseMIT

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-attributes

However, unlikeexceptions.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.

@carsonbotcarsonbot added this to the7.1 milestoneDec 22, 2023
@OskarStarkOskarStark changed the title[HttpKernel] Allow WithHttpStatus to take effect on interfaces[HttpKernel] Allow#[WithHttpStatus] to take effect on interfacesDec 22, 2023
@smnandre
Copy link
Member

How would that work with the exception mapping already handled ? Would there not be conflicts / priority troubles there ?

@priyadi
Copy link
ContributorAuthor

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 ofclass_implements(). There should be no BC break.

@smnandre
Copy link
Member

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
Copy link
ContributorAuthor

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

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

smnandre reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Looking at the implementation, I think this might be too unusual - ie unexpected. The relationship is unnatural to me.

Why don't you leverageHttpExceptionInterface instead?

@priyadi
Copy link
ContributorAuthor

Looking at the implementation, I think this might be too unusual - ie unexpected. The relationship is unnatural to me.

Why don't you leverageHttpExceptionInterface instead?

That would require my domain model to depend on HttpKernel.

Without this change, I could still do it usingconfig/packages/exceptions.yaml which works with interfaces. But I don't see why it should work usingconfig/packages/exceptions.yaml but should not work with attributes.

smnandre reacted with thumbs up emoji

@priyadipriyadi changed the title[HttpKernel] Allow#[WithHttpStatus] to take effect on interfaces[HttpKernel] Allow#[WithHttpStatus] and#[WithLogLevel] to take effect on interfacesDec 24, 2023
@priyadi
Copy link
ContributorAuthor

Added more background information in the PR description.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Got it, LGTM!

@nicolas-grekas
Copy link
Member

Thank you@priyadi.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dunglasdunglasdunglas approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

5 participants

@priyadi@smnandre@nicolas-grekas@dunglas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp