[5.6] Add a HTTP cache middleware #22389

dunglas · 2017-12-11T18:28:32Z

This is a port of the API Platform's HTTP cache listener.

It allows to easily set HTTP cache headers through a dedicated middleware:

Route::get('/my-route', function () {
    return view('welcome');
})->middleware('cache:max_age=180;s_maxage=60;etag;immutable');

With this config, the following Cache-Control header will be added: max-age=180, public, s-maxage=60 (180 seconds of client-side cache, can be cached by proxies during 60 seconds) and a E-Tag will be generated.

browner12 · 2017-12-11T18:33:59Z

better as a package?

dunglas · 2017-12-11T18:37:31Z

@browner12, it looks like a basic web framework feature to me (Symfony has something similar, Django too) but as I'm new to Laravel contribution, I don't know where it should belong.

Simperfit · 2017-12-11T20:39:52Z

src/Illuminate/Http/Middleware/Cache.php

+    public function handle($request, Closure $next, int $maxAge = null, int $sharedMaxAge = null, bool $public = null, bool $etag = false)
+    {
+        /**
+         * @var   \Symfony\Component\HttpFoundation\Response


missing $response

It has been removed by StyleCI actually (29a3e96#diff-b3ff80d2f50088f0bfe3b4e15d654af9L23). It looks like a bug.

browner12 · 2017-12-11T21:26:04Z

my experience with HTTP caching headers has been on the web server. not sure if it's a good idea or not to have it at the app level.

the only reason I suggested it as a package, is historically Taylor kept these types of things out of core and suggests it that way, but I'm not necessarily opposed to it in core.

sisve · 2017-12-12T06:29:23Z

src/Illuminate/Http/Middleware/Cache.php

+        }
+
+        if (! $response->getContent()) {
+            return;


This looks weird. Shouldn't we always return something from middlewares?

sisve · 2017-12-12T06:32:05Z

src/Illuminate/Http/Middleware/Cache.php

+            return;
+        }
+        if ($etag) {
+            $response->setEtag(md5($response->getContent()));


How do I set a custom etag?

sisve · 2017-12-12T06:42:01Z

This looks incomplete. We would still need a middleware that checks the If-None-Match header if the etag is still valid and return a 304 Not Modified. With this current implementation we would still need to build the entire response to evaluate if the etag is valid.

How do I set Cache-Control: ... no-cache?
How do I set Cache-Control: ... no-store?
How do I set Cache-Control: ... must-revalidate:
How about supporting Expires, Last-Modified?
How about building that middleware that validates etags and returns a 304 Not Modified?
How about building that middleware so it also supports the If-Modified-Since related to point 4?

dunglas · 2017-12-12T07:14:20Z

Indeed it doesn't support 100% of the spec. It is a deliberate choice because it's not possible to use named arguments for a middleware, and middlewares with a lot of arguments are not really practical.

An alternative would be to use the setCache() method provided by the Symfony response that takes an array of options into parameter. It would be easier to use, but it requires to introduce a new syntax like:

Route::get('/my-route', function () {
    return view('welcome');
})->middleware('cache:max_age=180;s_maxage=60;etag=abc;immutable=true');

Do you think such syntax is acceptable?

About your specific questions:

How do I set Cache-Control: ... no-cache

It's the default value hardcoded in the Symfony Response.

How do I set Cache-Control: ... no-store, must-revalidate

It is not supported (see initial comment) but if we want to support them, I can add new flags for no-cache and those ones.

How about supporting Expires, Last-Modified

I've explicitly not supported them because max-age and e-tag should respectively be preferred to those ones (they prevent problems with clock errors).

How about building that middleware that validates etags and returns a 304 Not Modified?

Yes, for now it's delegated to the reverse proxy but I plan to open another PR with such middleware when this one will be ready.

Support for immutable is missing too. I'll add it because this one has a real value.

sisve · 2017-12-12T08:27:01Z

Indeed it doesn't support 100% of the spec. It is a deliberate choice because it's not possible to use named arguments for a middleware, and middlewares with a lot of arguments are not really practical.

I've explicitly not supported them because max-age and e-tag should respectively be preferred to those ones (they prevent problems with clock errors).

Yes, for now it's delegated to the reverse proxy but I plan to open another PR with such middleware when this one will be ready.

I believe that this part is mega-super-important. There's deliberate choices taken, but those were initially totally undocumented. There were nothing in the PR description that says "I decided to do this partially, some things are done, other parts have to be added laters".

So this PR is admittedly incomplete since it lacks a middleware to handle the requests, and you are expected to run a reverse proxy in front of nginx to have this work. There's nothing in this PR on how to setup that proxy, and we would need an entire new section in the docs for this. (Or handle the wrath of the support questions about how to do it; "Hey, I added the Cache middleware but nothing is cached?")

I've explicitly not supported them because max-age and e-tag should respectively be preferred to those ones (they prevent problems with clock errors).

This is a decision that means that we have to regenerate the entire dynamic response to md5-hash it to check if it has changed. In other terms, it saves nothing at all for the origin server, it only reduces network bandwidth... except that this PR doesn't do that either. We're expected to setup a reverse proxy ourselves.

The current implementation should be renamed to "SetSomeCachingHeaders", because that is all it does.

 An origin server SHOULD send Last-Modified for any selected
representation for which a last modification date can be reasonably
and consistently determined, since its use in conditional requests
and evaluating cache freshness ([RFC7234]) results in a substantial
reduction of HTTP traffic on the Internet and can be a significant
factor in improving service scalability and reliability.

First paragraph in RFC 7232 about generation of Last-Modified.

I know that I am old and bitter; but you're not suggesting things to a third party package that people may opt-in to use. You're suggesting things into a framework that has at least half a million installations (quick search at builtwith.com). We need complete implementations, lots of QA and be certain that what is merged is correct and maintainable.

dunglas · 2017-12-12T08:29:04Z

I know that I am old and bitter; but you're not suggesting things to a third party package that people may opt-in to use. You're suggesting things into a framework that has at least half a million installations (quick search at builtwith.com). We need complete implementations, lots of QA and be certain that what is merged is correct and maintainable.

I've opened this PR to discuss it, don't worry ;)

dunglas · 2017-12-12T08:59:26Z

Here is a new implementation:

more expressive (but introduces a new syntax, not sure if it's OK or not)
rely directly on Symfony's HttpFoundation's setCache, so supports all the spec, and any new option we implement will be automatically supported by Laravel
generate a 304 when appropriate

WDYT @sisve

dunglas · 2017-12-12T09:14:51Z

Btw:

First paragraph in RFC 7232 about generation of Last-Modified.

From the same RFC:

If a response includes a Cache-Control field with the max-age
directive (Section 5.2.2.8), a recipient MUST ignore the Expires
field. Likewise, if a response includes the s-maxage directive
(Section 5.2.2.9), a shared cache recipient MUST ignore the Expires
field. In both these cases, the value in Expires is only intended
for recipients that have not yet implemented the Cache-Control field.

And according to MDN, all modern browsers support Cache-control, so in 2017 it's totally useless to support Expires (but the new implementation does it, anyway).

And regarding the PR description, I say "easily set HTTP cache headers", it doesn't say that it adds a cache layer. It's basic HTTP knowledge that cache headers must be handled by clients and or proxies. Anyway, the new implementation automatically sets the 304 status code when appropriate (to save bandwidth), and allow to set manually a etag.

sisve · 2017-12-12T10:20:49Z

I believe we have different views on how much the average Laravel developer knows about "basic HTTP knowledge", but that could just be my usual bitterness talking.

I prefer this newer implementation, mostly because it actually does what can be expected from a middleware. I like that the parameters are optional, and if I don't specify them I will fallback to what the controller provided in the response. This allows me to build custom etags or Last-Modified-dates. There's one weird thing left; why can't we cache empty contents? Wouldn't that mean that we cannot cache a 204 No Content? On the other hand, when is that a reasonable response at all from a GET/HEAD...

dunglas · 2017-12-12T10:49:22Z

We had some WTF with caching responses containing only headers, but indeed this check can be removed.

GrahamCampbell · 2017-12-12T14:18:19Z

This is a port of the API Platform's HTTP cache listener.

Why do we need to copy their code? Why not load the package in?

GrahamCampbell · 2017-12-12T14:18:34Z

TBH, I do feel this is better off as a laravel package, and not in the core.

tillkruss · 2017-12-12T14:24:55Z

Same here.

taylorotwell · 2017-12-12T14:52:35Z

@dunglas thanks for the PR. What made you want to PR this into Laravel? Are you working on some feature related to Laravel that needed this?

dunglas · 2017-12-12T20:14:50Z

Hi folks,

@GrahamCampbell the API Platform's listener works with the Symfony kernel, it is not (yet) compatible with Laravel.

@taylorotwell I'm working on a Laravel project and have to set some cache headers on a per route-group basis.
I didn't find an easy (builtin way) to do this. As some other frameworks have builtin support for this feature, I opened this PR because I think it's a common need.
The use case is: set with ease a HTTP cache policy, just like the auth middleware allows to set an authorization strategy.

browner12 · 2017-12-12T20:27:08Z

if these are set, do they take precedence over caching headers set on the web server?

dunglas · 2017-12-12T22:12:18Z

@browner12 it depends of the configuration of your web server / proxy.
Nginx will not by default, but you can force it to replace them using headers-more.
With Varnish, you can do whatever you want using a VCL.

Generally speaking, it's better to let the application managing the caching strategy, because it have to more data and logic than the web server.

browner12 · 2017-12-12T22:27:27Z

oh gotcha. I think I was getting this confused with setting cache headers for assets like CSS, JS, SVG, etc. This would be caching the response (html page)?

dunglas · 2017-12-12T23:14:49Z

This will tell the client (max_age) or proxies (s_maxage) to cache the response. There is no builtin cache layer (it should be possible to add something like https://symfony.com/doc/master/http_cache.html#symfony-reverse-proxy, but it will always be slower than a dedicated tool coded in C such as Varnish).

taylorotwell · 2017-12-13T14:05:02Z

I'm generally OK with this since it doesn't really affect default applications and is opt-in.

taylorotwell · 2017-12-13T14:13:33Z

Renamed to SetCacheHeaders. Thanks!

barryvdh · 2017-12-13T19:30:01Z

You should be able to cache it with https://github.com/barryvdh/laravel-httpcache whichs uses he mentioned Symfony caching :)

dunglas changed the title ~~Add an HTTP cache middleware~~ Add a HTTP cache middleware Dec 11, 2017

Simperfit reviewed Dec 11, 2017

View reviewed changes

tillkruss changed the title ~~Add a HTTP cache middleware~~ [5.6] Add a HTTP cache middleware Dec 11, 2017

sisve reviewed Dec 12, 2017

View reviewed changes

dunglas added 2 commits December 12, 2017 09:56

Add an HTTP cache middleware

5defbd7

Rely on setCache

6a1480a

dunglas force-pushed the http-cache-middleware branch from 29a3e96 to 6a1480a Compare December 12, 2017 08:57

Fix bug

9d6001b

dunglas force-pushed the http-cache-middleware branch from bb3cb97 to 9d6001b Compare December 12, 2017 09:01

taylorotwell merged commit 9d6001b into laravel:master Dec 13, 2017

dunglas deleted the http-cache-middleware branch December 13, 2017 14:15

dunglas mentioned this pull request Dec 18, 2017

[5.6] Add the SetCacheHeaders middleware laravel/laravel#4515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.6] Add a HTTP cache middleware #22389

[5.6] Add a HTTP cache middleware #22389

dunglas commented Dec 11, 2017 •

edited

Loading

browner12 commented Dec 11, 2017

dunglas commented Dec 11, 2017 •

edited

Loading

Simperfit Dec 11, 2017

dunglas Dec 11, 2017 •

edited

Loading

browner12 commented Dec 11, 2017

sisve Dec 12, 2017

sisve Dec 12, 2017

sisve commented Dec 12, 2017

dunglas commented Dec 12, 2017 •

edited

Loading

sisve commented Dec 12, 2017

dunglas commented Dec 12, 2017

dunglas commented Dec 12, 2017 •

edited

Loading

dunglas commented Dec 12, 2017 •

edited

Loading

sisve commented Dec 12, 2017

dunglas commented Dec 12, 2017

GrahamCampbell commented Dec 12, 2017

GrahamCampbell commented Dec 12, 2017

tillkruss commented Dec 12, 2017

taylorotwell commented Dec 12, 2017

dunglas commented Dec 12, 2017

browner12 commented Dec 12, 2017

dunglas commented Dec 12, 2017 •

edited

Loading

browner12 commented Dec 12, 2017

dunglas commented Dec 12, 2017

taylorotwell commented Dec 13, 2017

taylorotwell commented Dec 13, 2017 •

edited

Loading

barryvdh commented Dec 13, 2017

[5.6] Add a HTTP cache middleware #22389

[5.6] Add a HTTP cache middleware #22389

Conversation

dunglas commented Dec 11, 2017 • edited Loading

browner12 commented Dec 11, 2017

dunglas commented Dec 11, 2017 • edited Loading

Simperfit Dec 11, 2017

Choose a reason for hiding this comment

dunglas Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

browner12 commented Dec 11, 2017

sisve Dec 12, 2017

Choose a reason for hiding this comment

sisve Dec 12, 2017

Choose a reason for hiding this comment

sisve commented Dec 12, 2017

dunglas commented Dec 12, 2017 • edited Loading

sisve commented Dec 12, 2017

dunglas commented Dec 12, 2017

dunglas commented Dec 12, 2017 • edited Loading

dunglas commented Dec 12, 2017 • edited Loading

sisve commented Dec 12, 2017

dunglas commented Dec 12, 2017

GrahamCampbell commented Dec 12, 2017

GrahamCampbell commented Dec 12, 2017

tillkruss commented Dec 12, 2017

taylorotwell commented Dec 12, 2017

dunglas commented Dec 12, 2017

browner12 commented Dec 12, 2017

dunglas commented Dec 12, 2017 • edited Loading

browner12 commented Dec 12, 2017

dunglas commented Dec 12, 2017

taylorotwell commented Dec 13, 2017

taylorotwell commented Dec 13, 2017 • edited Loading

barryvdh commented Dec 13, 2017

dunglas commented Dec 11, 2017 •

edited

Loading

dunglas commented Dec 11, 2017 •

edited

Loading

dunglas Dec 11, 2017 •

edited

Loading

dunglas commented Dec 12, 2017 •

edited

Loading

dunglas commented Dec 12, 2017 •

edited

Loading

dunglas commented Dec 12, 2017 •

edited

Loading

dunglas commented Dec 12, 2017 •

edited

Loading

taylorotwell commented Dec 13, 2017 •

edited

Loading