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

FEATURE: Tooling for working with response headers in controller #3291

Open
Tracked by #2156
mhsdesign opened this issue Jan 29, 2024 · 0 comments
Open
Tracked by #2156

FEATURE: Tooling for working with response headers in controller #3291

mhsdesign opened this issue Jan 29, 2024 · 0 comments

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Jan 29, 2024

Initially the idea was was part of #2219 but reverted and i reverted the commit which makes it a reimplementation here: #3290

Maybe the ActionResponse Headers is stretching things a bit... have removed this for now and will create another PR suggesting to change to that. I do think the Headers class handles some cases for abstracting headers very well (e.g. Cache-Control and date based headers like Last-Modified) and should be the underlying ActionResponse API

Or differently expressed in #2156 (comment) by @albe:

One idea: Let's move the currently deprecated Http\Headers class into the Mvc namespace and make it part of the ActionResponse API in order to specify headers to set in the response [...].
The idea is that ActionResponse is a higher level abstraction of a response and Headers is a (slightly) higher level abstraction of basic Http headers that hides some details like the Cache-Control stuff.


So i dug a bit into the history and it seem the Header class was deprecated in #1366 (Flow 5.1)

Previously it was used in the legacy (pre prs7) \Neos\Flow\Http\Response and even shortly in the new ActionResponse #1531 before psr7.

The Header class went with #2626 through its last refactoring but wasnt touched really since.

Currently its only used as storage for \Neos\Flow\Http\Client\Browser::addAutomaticRequestHeader which is no real usecase. And even partially broken. The Header class is as good as dead.


After looking a bit into the Header class it might really well good that this is not super heavily in use. There are lots of tests, but the implementation looks rather complex, and there are missing features like case-insensitive header name matching.

Maybe we need a another utility / abstraction but just not the Header again :D
Possibly even more utilities similar to the Header's \Neos\Flow\Http\CacheControlDirectives would be a good fit.
The CacheControlDirectives should according to the docs be already preferred over the Header class and might be used like:

$cacheControlHeaderValue = $response->getHeaderLine('Cache-Control');
$cacheControlDirectives = CacheControlDirectives::fromRawHeader($cacheControlHeaderValue);
$cacheControlDirectives->setDirective('max-age', 3600);
$cacheControlHeaderValue = $cacheControlDirectives->getCacheControlHeaderValue();
$response = $response->withHeader('Cache-Control', $cacheControlHeaderValue);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant