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

Unnecessary extra headers send #626

Closed
joanhey opened this issue Feb 16, 2025 · 7 comments
Closed

Unnecessary extra headers send #626

joanhey opened this issue Feb 16, 2025 · 7 comments

Comments

@joanhey
Copy link
Contributor

joanhey commented Feb 16, 2025

Actually always add Cache-control and Expires headers.

If there is no Cache-Control header and no Expires header, and no Last-Modified header (which most web servers send by default for static assets) is never cached by the browser.

And Cache-Control (http 1.1) have preference over Expires (http 1.0)

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Expires

Note: If there is a Cache-Control header with the max-age or s-maxage directive in the response, the Expires header is ignored.

Any app using an API with Flight, also don't need these headers.

@n0nag0n
Copy link
Collaborator

n0nag0n commented Feb 16, 2025

Are you talking about this section of code specifically?

https://github.com/flightphp/core/blob/master/flight/net/Response.php#L434-L438

I guess it'd be helpful to understand why those headers should just not be sent at all? I seem to recall needing to add them because otherwise API calls were mysteriously caching when they shouldn't have been even though different data sets were being sent.

@n0nag0n
Copy link
Collaborator

n0nag0n commented Feb 16, 2025

Also at this point you should just become an honorary member of flightphp with all the Issues and PR's you're helping with hahaha

@n0nag0n
Copy link
Collaborator

n0nag0n commented Feb 16, 2025

I found the original commit/PR, and yeah that's the reason why I had to add those.

#598

@joanhey
Copy link
Contributor Author

joanhey commented Feb 16, 2025

Absolute none framework add that headers.

But remember no headers at all !!
Not Cache-control, Expires, Last-Modified and Pragma !!

Test it in local.

PD: if you add only 1 of these headers, then start the problem.

And please don't add the Content-Length, that's automatic in normal PHP.

@joanhey
Copy link
Contributor Author

joanhey commented Feb 16, 2025

All that headers added to the response, are terrible for any benchmark !!
And for any app, the response only need to return the data and necessary headers.

@joanhey
Copy link
Contributor Author

joanhey commented Feb 17, 2025

I can be an honorary member, but it's better if the people follow me, or help like sponsor.

@n0nag0n
Copy link
Collaborator

n0nag0n commented Feb 21, 2025

I did what you suggested and it looks like it's working as expected. My guess is that previously some header was added for caching elsewhere and it's since been fixed. I'll take the performance win! Thanks!

#630

@n0nag0n n0nag0n closed this as completed Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants