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

Truncating response body #55

Open
wants to merge 3 commits into
base: 1.x
Choose a base branch
from

Conversation

JimTools
Copy link

@JimTools JimTools commented Mar 7, 2024

truncating the body to reduce data transmission on cache hits.

closes #54

Adding tests ETag and Last-Modified to ensure the
response body shouldn't send data when their is a
cache hit.
Replacing the response body on cache hits to reduce data transmission
$this->type = $type;
$this->maxAge = $maxAge;
$this->mustRevalidate = $mustRevalidate;
$this->streamFactory = $streamFactory;
Copy link
Author

Choose a reason for hiding this comment

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

Maybe for now it's better to add slim/psr7 and call StreamFactory this would make it easier for anyone already using this package?

Copy link
Contributor

@flavioheleno flavioheleno Jul 7, 2024

Choose a reason for hiding this comment

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

I'd rather require psr/http-factory (and likely psr/http-factory-implementation) than slim/psr7 as it keeps interoperable with other psr7 implementation packages

Copy link
Contributor

Choose a reason for hiding this comment

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

Only the user should decide which StreamFactoryInterface implementation should be used in their application.

Copy link
Author

Choose a reason for hiding this comment

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

@flavioheleno @odan thank you for taken the time to review the PR I’ll update when I get some time.

src/Cache.php Outdated
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;

use Slim\Psr7\Factory\StreamFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency is not needed.

@odan
Copy link
Contributor

odan commented Jul 9, 2024

I've been thinking about this approach. Wouldn't it be easier to implement a custom middleware that checks the response for status code 304 and, if the case, returns a response with an empty body. The advantage would be that this can be added when needed and doesn't break the existing logic.

@flavioheleno
Copy link
Contributor

Honestly, dropping the body is something that I'd expect from this middleware and not have to add another one to handle it.
IMHO, the current behavior/logic is broken.

Removig StreamFactory as it's not used.
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

Successfully merging this pull request may close these issues.

Body is still sent on 304
3 participants