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 dynamic header values within the WithHttpStatus attribute #48890

Closed
wants to merge 2 commits into from

Conversation

angelov
Copy link
Contributor

@angelov angelov commented Jan 5, 2023

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

This extends the functionality provided with the #[HttpStatus] #[WithHttpStatus] attribute to be allowing the header values to be set dynamically.

With the current implementation, the header values must be hard-coded:

#[WithHttpStatus(
    statusCode: 422,
    headers: [
        'max-allowed-amount' => 500,
    ]
)]
class AmountTooBigException extends \Exception
{
    public function __construct(public readonly int $maxAllowedAmount)
    {
    }
}

With this, we can provide an Symfony\Component\ExpressionLanguage\Expression object as value for any of the headers, which will be evaluated when preparing the response.
Note that the exception object can be accessed in the expression by using this, as in the example bellow.

#[WithHttpStatus(
    statusCode: 422,
    headers: [
        'max-allowed-amount' => new Expression('this.maxAllowedAmount'),
        'hard-coded-value' => 'still-possible',
    ]
)]
class AmountTooBigException extends \Exception
{
    public function __construct(public readonly int $maxAllowedAmount)
    {
    }
}

Note that this requires the ExpressionLanguage so it is added as a suggested package.
To be honest, I am not really sure if this is useful enough to add a new optional dependency, but I wanted to hear more opinions.

@carsonbot
Copy link

Hey!

I think @HeahDude has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@angelov
Copy link
Contributor Author

angelov commented Jan 6, 2023

The change requested by fabbot is not related:

-            KazInfoTehTransportFactory::class => 'notifier.transport_factory.kaz-info-teh',
+            KazInfoTehTransportFactory::class => 'notifier.transport_factory.kaz-info-the',

https://fabbot.io/patch/symfony/symfony/48890/71d5b22d86d0da2be2b1542bdd45207322a013fb/typos.diff

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.

The implementation LGTM but I don't really have an opinion whether we need this.
An alternative is to implement HttpExceptionInterface, at the cost of some coupling with HttpKernel.
Another approach could be to allow headers to be a string, which would then be used as the name of the method that returns the headers.

@nicolas-grekas
Copy link
Member

Let me close this because we didn't get much traction here. Let's reconsider if one has a real world need.
Thanks for submitting!

herberterich

This comment was marked as spam.

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

4 participants