Skip to content

[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

Merged
merged 1 commit into from
Dec 27, 2023
Merged

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

merged 1 commit into from
Dec 27, 2023

Conversation

priyadi
Copy link
Contributor

@priyadi priyadi commented Dec 22, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

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 like config/packages/exceptions.yaml but using attributes: https://symfony.com/blog/new-in-symfony-6-3-http-exception-attributes

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

@carsonbot carsonbot added this to the 7.1 milestone Dec 22, 2023
@OskarStark OskarStark changed the title [HttpKernel] Allow WithHttpStatus to take effect on interfaces [HttpKernel] Allow #[WithHttpStatus] to take effect on interfaces Dec 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
Contributor Author

priyadi commented Dec 23, 2023

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 class_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 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;
}

@priyadi
Copy link
Contributor Author

priyadi commented Dec 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

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
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 leverage HttpExceptionInterface instead?

@priyadi
Copy link
Contributor Author

priyadi commented Dec 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 HttpExceptionInterface instead?

That would require my domain model to depend on HttpKernel.

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

@priyadi priyadi changed the title [HttpKernel] Allow #[WithHttpStatus] to take effect on interfaces [HttpKernel] Allow #[WithHttpStatus] and #[WithLogLevel] to take effect on interfaces Dec 24, 2023
@priyadi
Copy link
Contributor Author

priyadi commented Dec 24, 2023

Added more background information in the PR description.

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.

Got it, LGTM!

@nicolas-grekas
Copy link
Member

Thank you @priyadi.

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.

5 participants