One of the big intended benefits from the Kernel work is the ability to use Http caching logic directly rather than implementing our own caching logic. The Kernel component includes a PHP-space implementation of an Http reverse proxy cache. We want to use that for all kernel-called page segments; that is, the entire page itself plus all of the sub-requests. The easiest way to do that is to move the kernel from the 'kernel' DI entry to some other entry, and then make the 'kernel' entry an instance of the HttpCache object, configured with a Store class of our own that saves stuff to the Drupal cache system.
For now, we should let the HttpCache use whatever logic it uses. We can refine it later after the pieces are in place.
Of course, we'll also want a way to disable that cache object when a site is running behind a real proxy cache like Varnish, which is going to be way faster anyway.
This probably needs to wait for #1595146: Load the HttpKernel from the DI Container
Comment | File | Size | Author |
---|---|---|---|
#90 | 1597696-use-symfony-http-cache.patch | 17.77 KB | znerol |
#70 | drupal-httpcache-7269676-70.patch | 17.68 KB | iamEAP |
#64 | 1597696-httpcache-2.patch | 17.67 KB | Crell |
#63 | 1597696-httpcache.patch | 41.47 KB | Crell |
#44 | 1597696-44-http-cache.patch | 9.17 KB | Anonymous (not verified) |
Comments
Comment #1
Crell CreditAttribution: Crell commentedTagging.
Comment #2
catchThis needs benchmarks, we may well want to do it even if there's a regression, but let's not have another nasty surprise like #1064212: Page caching performance has regressed by 30-40% in D7 (which was due to a very tiny, innocent patch).
I'm also interested to see how this could tie in with cache tag support. We don't have proper content-based cache tags attached to cache items yet, but there's proof of concept code that integrates with cache tags with varnish already, so if varnish can handle it, then HttpCache should be able to as well.
Comment #3
Crell CreditAttribution: Crell commentedAgreed entirely with #2. I have no idea how cache tag support could tie in here, frankly, but it should be investigated. :-)
That said, there are key differences between the way Drupal has traditionally cached things and the way Symfony assumes you'll cache things.
In Drupal, we assume "cache forever, but flush often". That emphasizes data freshness over performance or cache efficency, and results in less useful caches.
Symfony, from what I've seen, encourages "cache briefly, don't bother flushing." That emphasizes cache efficiency over data freshness, at the expense of some rendered information being briefly stale.
I don't believe we should switch over to "cache briefly, don't flush" wholesale; however, we should consider where that is a more effective strategy; if the code is simplier and more stable and more reliable if we cache for 5 seconds and accept that "meh, this view could be stale for 5 seconds, whatever", is that OK? Where is that OK?
I don't know, but we should be asking that.
Comment #4
catchI think that's a site-specific question. You can currently set Drupal up to take either of these approaches via contrib and/or configuration, and we should be careful not to close either of them off to much (for example if you don't want content to clear on cache tags, it's 5 minutes to write a cache backend that ignores them).
Comment #5
Crell CreditAttribution: Crell commentedWell, then we need to make it code-free to adjust your caching strategy, at least within reason. That won't be a simple task. :-)
Either way, I think the first implementation should be kept as simple as possible and just restructure the code to use/not use HttpCache wrapped around the kernel.
Comment #6
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedBy cache tags you mean a unique identifier for the content, so that if the tags are equal, then the content does not need to be regenerated? HTTP (and Symfony2) supports the validation approach of caching via ETags.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedre #6, cache tags as in the cache tags support in Drupal's cache API - http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...
Comment #8
msonnabaum CreditAttribution: msonnabaum commentedHere's a rough POC that I've been working on.
This should replace our default page caching with the file-based HttpCache. We'll obviously need to make additional storage backends, but I'm not against changing our default MySQL backend for flat files.
The biggest problem I see thus far is that the earliest we can serve a page from cache is after DRUPAL_BOOTSTRAP_CODE. This is much later than I'd like, but as long as we know there's work to be done to instantiate the HttpKernel earlier, this shouldnt be a blocker.
Comment #9
Crell CreditAttribution: Crell commentedbot snack?
Comment #11
Crell CreditAttribution: Crell commentedAs soon as the bundles patch goes in, which I believe puts the config system into the DIC, we should start injecting it and eliminating the use of the config() wrapper function.
I especially like how this is 2/3 removing code rather than adding it. :-) Is that because the existing Symfony code does what we were doing, or because you just didn't get to that part yet?
Comment #12
msonnabaum CreditAttribution: msonnabaum commentedAgreed.
A little of both. Its hard to follow at the moment because we left in a bunch of code that calls header() directly after the initial kernel patch. In this case it was easier to just rip that code out and get the cache working as we'd expect it to. A lot of what is removed is handling around 304s, which I think we're getting already. It's possible that we'll need to use HttpCache's validation to do some additional 304 checking.
Comment #13
Crell CreditAttribution: Crell commentedGotcha. I'm OK with a temporary regression in the elegance of our 304 handling while we suss out what if anything we need to do atop Symfony's existing support for that.
The original kernel patch had a target of "keep tests passing", so there's definitely some needlessly redundant header() calls lying about still. I think those should all be eliminated entirely.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedBundles patch is in. Hoping someone can get to this soon.
Comment #15
R.Muilwijk CreditAttribution: R.Muilwijk commentedThe patch is not what I would expect. The HttpCache is added at the end just before the kernel handle's the request. This is after the complete bootstrap. Shouldn't the cache be utilized in the DRUPAL_BOOTSTRAP_PAGE_CACHE phase. Is this even possible with the Kernel + HttpCache at that early stage?
Comment #16
R.Muilwijk CreditAttribution: R.Muilwijk commentedAnother thing we have to think about is the File Store because we can't assume we can use /tmp. How can we retrieve the file store location without getting the configuration from db?
Comment #17
pounard@#15 I guess that full bootstrap is slowly being freed from its fat while services are being ported into the DIC. I hope that in stable 8.x bootstrap will only be a matter of a few millisec.
Comment #18
pounard@#16 I think that this kind of early needed configuration variables should be set in settings.php (pretty much like advanced cache backends configuration).
Comment #19
R.Muilwijk CreditAttribution: R.Muilwijk commentedWhat would be the proper solution to be able to override HttpCache and Store in for examply your settings.php?
Comment #20
pounardI don't think I can answer that much, I'm, just like you, waiting to see a working patch to know :)
Comment #21
R.Muilwijk CreditAttribution: R.Muilwijk commented@#17 so should we just aim for removing the DRUPAL_BOOTSTRAP_PAGE_CACHE completely?
@#18 Then settings.php should be changed to have the configuration in it and it should be set during install. Also what to do with the UI where you can set the temporary directory?
@#20 I'll do something similar like the cache_backends:
Comment #22
R.Muilwijk CreditAttribution: R.Muilwijk commentedMy latest version of the patch. Lets meet up at the sprint.
Comment #23
R.Muilwijk CreditAttribution: R.Muilwijk commentedForgot to add the HttpCache.
Comment #24
R.Muilwijk CreditAttribution: R.Muilwijk commentedSo these are the things I think we need to sort out:
UPDATE: added 11
Comment #25
iamEAP CreditAttribution: iamEAP commentedOn 11: D7 stores cache_page (and cache_block) entries as CACHE_TEMPORARY, even with minimum cache lifetime.
If HttpCache assumes a ttl, it sounds like we may be scrapping cache minimum for cache maximum.
Comment #26
aspilicious CreditAttribution: aspilicious commentedTriggering bot
Formatting issue
Comment #28
cosmicdreams CreditAttribution: cosmicdreams commentedWhy define $config here if we're not going to do anything with it?
Would be nice if we could group all included Symfony libraries and all Drupal libraries, Symfony first.
Another reason not to define $config within Drupal\Core\HttpCache if we're not going to use it. We're loading it again here.
Comment #29
msonnabaum CreditAttribution: msonnabaum commentedThanks to everyone who has recently reviewed this patch. Although many points brought up here are valid, I'd like to postpone most of them until we get the basics working with tests passing. For example, I'm purposefully not dealing with alternative stores atm. Just need to get the default working first. Also, being called so late in the bootstrap is a problem, but perhaps not this patch's problem. Let's revisit once it works.
Some changes in #1698108: Update Drupal's dependencies broke this patch pretty badly, so here's a quick reroll which I believe only takes those changes into account. Actual caching of pages seems to work correctly, but the tests that aren't passing are legit. We still need to figure out how and where to handle the logic around "page_cache_invoke_hooks".
Comment #31
msonnabaum CreditAttribution: msonnabaum commentedLet's try that again.
Comment #32
geerlingguy CreditAttribution: geerlingguy commentedComment #34
cosmicdreams CreditAttribution: cosmicdreams commentedif we intend to completely replace our caching mechanisms with the HttpCache, it might be beneficial to rip that out now and see what breaks. There seems to be a lot of issues involving the caching of user information. It's unclear to me what's the cause. With guidance I can try to reproduce the tests and focus on part of this patch.
Comment #35
msonnabaum CreditAttribution: msonnabaum commentedFound quite a few problems caused by my subclassing HttpCache from HttpKernel instead of from the framework bundle which the docs show. That said, there's parts of the framework bundle version that we don't want, so I just pulled in the relavant bits into our HttpCache class. Attached is a new patch that fixes some of this.
Unfortunately, I don't think we can fix the bootstrap hook issues with the way things are now. We need to move all of our bootstrap, with maybe DRUPAL_BOOTSTRAP_CONFIGURATION as an exception, inside the kernel. It's very awkward that we're mixing them right now and too much work is already done by the time we even instantiate the kernel.
This would allow us to fix the hook issues and it would also eliminate the performance regression I mentioned earlier, so it's worth doing asap. I'm considering this issue blocked until that is done.
Comment #36
catchWe can eventually replace all our HTML caching with HttpCache (i.e. page/block/drupal_render() in core), but there's plenty else that's not replaceable, and Symfony has plenty of its own caching (just renamed to compiling so it can pretend it's not).
@msonnabaum I'd consider having only one bootstrap path a critical task for Drupal 8 - it's OK if we have a small Drupal bootstrap before the kernel, but it's not really OK to have these two bootstraps mixed in with each other wrapping around.
Comment #37
msonnabaum CreditAttribution: msonnabaum commentedI agree with catch that we should move it all in, but it turned out that it was a tiny change to get what I need and also not break anything by leaving DRUPAL_BOOTSTRAP_CONFIGURATION out.
However, this has the rest of the bootstrap in the DrupalKernel, not the HttpCache wrapper. We all talked in IRC and were mostly in favor of removing the "normal" cache setting in favor of "aggressive" only. Supporting bootstrap hooks will be messy, so whether we can do it eventually or not, this is the simplest version start with.
Comment #38
msonnabaum CreditAttribution: msonnabaum commentedActually, as beejeebus pointed out to me, having the bootstrap in init() doesn't fix anything. It needs to go in boot().
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch to add #38.
Comment #41
Crell CreditAttribution: Crell commentedOn the assumption that this variable_get() will turn into a config object lookup, we should drop a @todo here noting that and that the config object should get injected when we get to that part.
In fact, that's probably the config object it should be on! :-)
Cookies and sessions should come from the $request. This is where the sessions->symfony issue comes in useful. If we can't do that yet because it's blocked on that patch, at least we should drop a @todo in here.
WAT? That could be why this won't install, perhaps?
Hm. I don't know how we can properly inject that config object. Probably it will have to just be hand-rolled in index.php. This far down, I think I'm OK with that.
-1 days to next Drupal core point release.
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch addresses #41.
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedspent some fun time* figuring out why 14k tests fail. the main problem seems to be this flow:
Drupal\Core\HttpCache::cacheEnabled --> config --> Drupal\Core\Config\Config::load --> Drupal\Core\Config\DatabaseStorage::read --> Drupal\Core\Config\DatabaseStorage::getConnection --> Drupal\Core\Database\Database::getConnection --> Drupal\Core\Database\Database::openConnection --> Drupal\Core\Database\Driver\mysql\Connection::__construct --> Drupal\Core\Database\Connection::__construct --> Drupal\Core\Database\Connection::setPrefix
this happens before _drupal_bootstrap_database(), so we haven't had a chance to munge global $databases with the test prefix, so all the db queries hit the parent drupal.
i've fixed that with a total hack of just calling drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE) in Drupal\Core\HttpCache::cacheEnabled(), which should let us see what the next series of fails are.
fixing the way we handle needing the database early in the request is out of scope for this issue.
* that may have been sarcasm.
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commenteda note for reviewers/testers: as well as turning on the page cache, you need to set a max lifetime.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedok, spent some time actually reading the Symfony HttpCache docs, and in the very helpful #symfony channel.
i don't think the pattern we've started with here is the right way to go, at all.
the symfony docs and the IRC channel make it clear we should only wrap our app kernel *if there's no http cache in front of drupal*.
so i guess we need to work out the best way to implement this pseudo-code for real:
Comment #48
Stof CreditAttribution: Stof commentedHi,
Just my 2 cents after the discussion with beejeebus on IRC to give the same knownledge to others.
Wrapping the kernel in the HttpCache will indeed be bad when you want to use Varnish, as it would continue to handle the ESI in PHP instead of letting Varnish doing it.
However, it is possible to check very easily if the HttpCache is needed. See https://gist.github.com/3551077
I haven't read the full discussion and I haven't checked the latest status of Drupal, so forgive me if the following advice is useless now.
When using the HttpCache, the instantiation of the DrupalKernel should be as lightweight as possible, as it will be done even when the cache is used (and so the drupal code does not need to be called). The heavy initialization should be done only when booting the kernel (which will occur only on cache miss, when the HttpCache needs to forward the request to the DrupalKernel).
If the instantiation of the kernel is heavy, you will have a bigger performance impact when switching between HttpCache and Varnish (as Varnish will also avoid the instantiation when the cache is used of course).
EDIT: here the code so that you don't need to go to the gist, as your comments support syntax highlighting
Comment #49
manarth CreditAttribution: manarth commentedThe hasSurrogateEsiCapability() function depends on headers from the proxy:
The ESI module already has a client-side JavaScript implementation for ESI, so there's a use-case where ESIs should be delivered, but there are no HTTP headers to indicate this.
I'd prefer to handle it via config instead.
Comment #50
pounard@manarth Could the JS add additional headers instead as part of the downgrade behavior?
Comment #51
Crell CreditAttribution: Crell commentedIf we wanted to use in-browser ESI, we should just use hInclude instead. Symfony already includes support for that in the HttpKernel from the FrameworkBundle, that we're already using.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedi've created #1766186: Move test prefix $databases munging earlier in the bootstrap to deal with the test-side global $databases munging.
with that patch, we should be able to call config() right after drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION), so i'm going to reroll with that patch included just to keep this moving.
things will shift further as config changes, but at least we can get the 'check config to see if we should wrap' pattern in, and fix the remaining fails.
Comment #53
Crell CreditAttribution: Crell commented#48 implies that we don't need a conditional. We just detect headers and we're done with it. What's the use case for doing otherwise? (JS-based pseudo-ESI is not one, because we can either add the headers we need or drop that entirely in favor of hInclude.)
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedESI or no ESI, who cares.
'just check http headers' is not going to fly as the sole determinant of 'should we wrap our kernel in HttpCache or not'.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm not working on this anymore, sorry if i've held it up.
Comment #56
msonnabaum CreditAttribution: msonnabaum commentedI completely agree that the "Surrogate-Capability" header is not enough to decide whether or not to use HttpCache. We should use either config or a $conf variable.
One thing that occurred to me while looking at this again however, is that our cache settings don't make much sense now that we've removed CACHE_TEMPORARY. We still have a cache.page.enabled option, yet it does nothing unless you set cache.page.max_age.
I'm thinking we should remove cache.page.enabled and replace it with http.cache.use.internal. If you are using a reverse proxy, all you have to do is set the max_age to something. I started on that but noticed we're checking the existing cache.page variables in some odd places, so I wanted to make sure we had consensus before making that change.
We should also conditionally pass the esi object in \Drupal\Core\HttpCache's constructor since we don't need that running if you don't have the equivalent of block_cache = TRUE. That can probably just be a @todo for now since removing block cache is out of scope for this issue.
Comment #57
Crell CreditAttribution: Crell commentedMark: So, you're saying request cache (which for now is synonymous with page cache, but later on won't be) is always-enabled, period, and to effectively disable it you just set the page 0 seconds? I'm OK with that. The default value should probably be something somewhat useful, say 5 min, but I'm easy on that part.
Or do I have it backward?
Comment #58
msonnabaum CreditAttribution: msonnabaum commentedI'm saying that the max age setting determines whether pages get cached. If it's at 0, drupal sends out the typical "Expires: Sun, 19 Nov 1978 05:00:00 GMT" that you'd get when page cache is off. If it's > 0, we send out the appropriate cache headers.
The only thing you explicitly enable is the internal page cache, which would mean we wrap the kernel with HttpCache.
Comment #59
Crell CreditAttribution: Crell commentedThat sounds fine to me.
Comment #60
catchI'd be fine with #58, we'll need to update the help text on the performance settings to reflect the change. Also fwiw completely fine with having the default max_age as 5 minutes (probably only in the standard profile).
Comment #61
Crell CreditAttribution: Crell commentedRelated: #1808080: Add an _internal route
Comment #62
msonnabaum CreditAttribution: msonnabaum commentedCreated #1853086: Remove cache.page.enabled in favor of an explicit internal cache setting for the changes I described in #58.
Comment #63
Crell CreditAttribution: Crell commentedGetting back to this at last...
Here's a new patch, mostly from scratch but with some code copied from Mark's patch above. For those playing along at home there's also a branch in the WSCCI sandbox. Overall, there should be more minus signs than plus signs, which is a good sign.
Changes:
- Removes the old page cache, and the bootstrap phase along with it. Ooo...
- Because there's no more page cache bootstrap phase, and the functions in cache.inc are now tiny, I moved them into bootstrap.inc and eliminated cache.inc. Ooo...
- Wrap the kernel in an HttpCache object, if a setting is set. It defaults to true.
- Sets cache headers that seem to be triggering the cache appropriately. I think. :-) It works in my testing, but could use more.
- Sets the headers conditionally, so if a response object from a controller already has cache-related headers set they don't get overridden. That way individual pages can opt-out of caching, or opt for a longer cache lifetime, or whatever else they feel like doing.
Still todo:
- Lots of testing.
- Figure out what happens to drupal_is_page_cacheable(). That sort of global approach totally won't work anymore. Best suggestion: Eliminate it outright and be done with it. If you want to mess with caching, that's what the response event is for. Fire after FinishResponseSubscriber and do whatever black magic you want.
- We should probably wire clearing the HttpCache page cache into drupal_flush_all_caches(), but I've not done so yet.
- Hook up the ESI support in HttpCache, which I've ignored for the time being.
Why???
- Because we want to have a single cache API all the way through, regardless of where it's getting cached. That cache API is Http. This lets a controller set a response object and headers on it, and they'll be honored regardless of where or how the response is being cached.
- Because we want to eliminate the "anonymous caches forever, authenticated never caches" distinction and move toward "things cache when they should cache, and don't when they shouldn't". This is a step in that direction.
Comment #64
Crell CreditAttribution: Crell commentedErm. One thing I forgot to mention. This does include the contents of #1945024: Remove subrequests from central controllers and use the controller resolver directly., because I expected it would matter. So far it doesn't.
Attached is a patch that contains just the cache changes, or should. :-)
Comment #65
msonnabaum CreditAttribution: msonnabaum commentedNone of this has anything to do with wrapping $kernel with HttpCache.
There are some worthwhile changes here, but it really should be handled outside of trying to use HttpCache. This issue should be about making page cache work with the new Request/Response objects.
Comment #66
katbailey CreditAttribution: katbailey commentedPretty sure we don't want the kernel booted for cached pages.
Comment #68
xjmComment #69
sdboyer CreditAttribution: sdboyer commentedlooking at the patch in #64, my primary comment is along the lines of katbailey's in #66 - if we're going to wrap the main kernel with the cache kernel, it needs to be done a little earlier. i'm not sure about booting/not booting the kernel, but i am sure that we should be slotting in this caching mechanism as early in bootstrap as possible, at least similar to where the old page caching implementation was. that may have implications on where/how we set the storage location var, but that shouldn't be awful -
sys_get_temp_dir()
is diiirty, but perhaps adequate for these purposes?beyond that, i agree with #65 - the rest of this issue is about fixing our interaction with Request/Response elsewhere in the code. fixing the failed tests should be a good start to that.
Comment #70
iamEAP CreditAttribution: iamEAP commentedRe-roll of #64.
Comment #71
Anonymous (not verified) CreditAttribution: Anonymous commentedare we dropping support for configuring caching from the UI?
Comment #72
Crell CreditAttribution: Crell commentedWe already have. This just moves around where things happen. And should probably be postponed on #1984766: Change notice: Start relying on Request/Response objects for cache handling
Comment #73
Anonymous (not verified) CreditAttribution: Anonymous commentedah, that will make things easier then.
i'll file a follow up to remove these values from core/modules/system/config/system.performance.yml:
and take those fields out of the performance form.
Comment #74
msonnabaum CreditAttribution: msonnabaum commentedTo clarify, we have NOT removed support for enabling cache from the UI, and I do not think we should.
Comment #75
Crell CreditAttribution: Crell commentedThen what was it we removed from the UI?
Comment #76
katbailey CreditAttribution: katbailey commentedIs this what you're referring to? #1853086: Remove cache.page.enabled in favor of an explicit internal cache setting
Comment #77
msonnabaum CreditAttribution: msonnabaum commentedYes, my patch only removed the "turn cache on" setting because it was redundant with max age. The setting is now "use internal cache", which you'd only check if you didn't have a reverse proxy cache.
Comment #78
Anonymous (not verified) CreditAttribution: Anonymous commentedok, so, #71 remains. using settings() is unpossible.
we need to use config() to check for 'use internal cache', which means we need a booted kernel.
Comment #79
Crell CreditAttribution: Crell commentedWell that's going to be a rather major problem, since avoiding a booted kernel is the whole point. :-) The toggle for invoking HttpCache needs to be readable before we boot anything or it's not useful.
Comment #80
katbailey CreditAttribution: katbailey commentedYeah, that would make me very sad. It's not clear to me why it would be so terrible to remove support for enabling the internal cache via the UI. Would be good to get some clarification on that.
Comment #81
msonnabaum CreditAttribution: msonnabaum commentedWhy are we talking about needing page cache to come before the kernel is booted? We're again trying to solve problems we don't have while we have many other caching related problems to work on.
Comment #82
msonnabaum CreditAttribution: msonnabaum commentedChanging the title to better reflect reality. This is still postponed and dependent on the rest of core using the kernel properly.
Comment #83
giorgio79 CreditAttribution: giorgio79 commentedDoctrine/commons is recommended here for caching instead of httpcache. Great read, take a look http://nerdpress.org/2012/07/10/caching-data-in-symfony2/
Comment #84
manarth CreditAttribution: manarth commentedThat article refers to using Doctrine/commons for caching data, rather than rendered pages/page fragments, and does acknowledge that for HTTP caching, the httpcache is fine.
Comment #85
catchComment #86
Crell CreditAttribution: Crell commentedI think we've determined that HttpCache isn't actually viable until we get rid of Simpletest and rewrite the installer, neither of which are happening in Drupal 8.
Very sad panda. :-(
Comment #87
chx CreditAttribution: chx commentedNote that ~/www/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpCache/HttpCache.php has an eval() in it which should be considered a blocker.
Comment #88
Wim Leers#2263981: Introduce a robust and extensible page cache-policy framework just landed.
According to #2257695: [meta] Modernize the page cache, this is now the next step. Not sure though, since this is now marked for 9.x-dev…
Comment #89
Crell CreditAttribution: Crell commentedIt was marked for 9.x when we didn't think we'd be able to get the bootstrap process sufficiently unravelled before 9.x. Turns out we did (neclimdul++), so it would be technically possible to use HttpCache now. I think it's still worth trying it out now that it's technically possible to do. It should be trivial to write a middleware for it to see what happens.
chx: That eval() does look a little suspect. Since the method isn't well commented I can't tell if the purpose of it is to protect against code injection or to specifically allow it in a special circumstance. I recommend asking upstream in Symfony about it. I would also be open to asking that the method be made protected so we can override it if necessary, but we should ask first.
Comment #90
znerol CreditAttribution: znerol commentedMoves the pieces into the right place, but currently fails because
session_name()
is set too late (should be fixed in #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator).Comment #91
znerol CreditAttribution: znerol commentedMoves the pieces into the right place, but currently fails because
session_name()
is set too late (should be fixed in #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator).Comment #93
Wim LeersShould this also remove some overhead and make Drupal 8 faster? I have no idea, I'm curious :)
This and other failures in
PageCacheTest
suggest thatHttpCache
doesn't follow the HTTP spec:… or that Drupal's current page cache doesn't, of course. At least it's a change in behavior that we should assess.
Comment #94
msonnabaum CreditAttribution: msonnabaum commentedWhy are we trying to fix something that's not broken again?
Comment #95
Crell CreditAttribution: Crell commentedBecause "broken" is not the issue. "Works" does not preclude "could be made better/simpler/less coupled". Also, HttpCache can do its own ESI tag handling whereas our current cache system does not, as far as I am aware.
Comment #96
msonnabaum CreditAttribution: msonnabaum commented*claps*
That's some masterful trolling there.
Comment #97
chx CreditAttribution: chx commentedWe are not adding another eval() into Drupal after years of work removed them. Just say no.
Especially not when
so postponed until at least we know more about that eval(). This status was made for it.
Comment #98
znerol CreditAttribution: znerol commentedThe
eval
is indeed ugly, but we can actually disable that part. Probably we'd need to do that in lookup as well.That said I'm okay with postponing this. Given that the page cache now lives in a stack middleware it is easy enough to swap it out.
Comment #99
Wim LeersThis is the offending code:
As you can tell, wonderfully undocumented. Googling for it only yields results that simply list source code. Zero documentation to be found.
Comment #100
dawehnerThis seems to be a case where the use of HTTP as standardtool even for internal documentation, falls down. On the other hand I don't even understand why this is needed.
Is there any reason why our system could not be expanded to do that?
Comment #101
catchPer #99 I looked at blame and this was added by Fabien Potencier in the initial commit of the cache system during 2010, was also unable to find any documentation.
HttpCache "doing it's own ESI tag handling" is a red herring and we should really examine what this means between HttpCache and Drupal's render caching and the various use cases. Have had enough of empty assertions constantly repeated.
Much of Symfony's use of ESI is to avoid having to deal with cache invalidation. They emphasise setting short expires on content listings so that they're 'fresh', then longer expires on the overall page. http://symfony.com/doc/current/book/http_cache.html has quite a long lead in, that boils down to "it's hard to do cache invalidation so we don't really bother but you can try if you want".
By implementing cache tags in Drupal 8, and specifically the bubbling of them from child elements to parents, we've almost entirely removed the need for different TTLs on cached content. All elements of the page, as well as the page itself, can have a very long TTL. When we invalidate, only parents of the cleared content get affected, children and siblings (etc.) can still be fetched from cache. Varnish has support for invalidating cache tags already since we put them in a custom header, so this also works with reverse proxies.
This means that for anonymous users, our cache hit rates should be considerably higher already than if we were using Symfony's short TTL + ESI tag approach - we don't need ESI to fix that particular performance issue, it's already solved with cache tags.
There's still things to clean up, like figuring out Etags #2259489: Leverage symfony for HTTP revalidation, but again HttpCache doesn't help with that at all.
Apart from different TTLs on content, the actual valid use case for ESI remaining is personalisation.
#post_render_cache is what we settled on for handling placeholders and replacement within HTML. Rather than using this for different TTLs, we only use that in Drupal for personalisation at the moment, because we just don't need different TTLs any more.
Since #post_render_cache replacement in PHP can be bypassed if necessary, it's possible to send the render cache placeholders to the reverse proxy or browser and replace them there instead of PHP (via Redis + memcache, Varnish or Akamai ESI tag with some massaging, or JavaScript). We don't need anything extra for actual ESI support except for ensuring this works and cleaning up various bits of work that are quite far along.
So the remaining question is whether we should add #post_render_cache support to the full page cache. With cache tag support, there is no value to doing this for anonymous users - we already have a viable model for all but the most extreme edge cases (and those can and should be handled by JavaScript usually).
What there could be value in doing though, is implementing 'full page caching' for authenticated users, then using #post_render_cache for personalisation. This means we'd no longer need to render the page template each time, which is one of the main places we could save time in HTML rendering that is not viable to cache for authenticated users. I think Fabianx has plans to do this, but again the benefit is for authenticated users - it'd be making the page cache viable for them too.
Another reason that Symfony's approach to ESI doesn't really help us, is that the recommended way to implement ESI is by doing subrequests inside Twig templates (see the docs at http://symfony.com/doc/current/book/http_cache.html#using-edge-side-incl...). That only works if you can decide beforehand where the dynamic content should be included in your template. I don't think that ESI caching is a decision that should be made in a Twig template.
See Wim, Marco and Fabian's talk here for a lot more detail: https://amsterdam2014.drupal.org/session/render-caching-drupal-7-and-8
Comment #102
Fabianx CreditAttribution: Fabianx commentedSo as Nat said, with using a placeholder based approach where placeholders are better defined than just post_render_cache random functions and everything does our own we can easily do:
- ESI
- AJAX
etc.
The advantage is that we can get data from the cache also directly, which is superior to internal micro ESI requests to assemble a page, which would not even propagate the render chain correctly.
The only advantage in my opinion is that HttpCache allows to save pages to disk, but that is something we can also have easily with the layer based system we have been building.
But we would need to extend it anyway to handle #attached, so in this case we gain not much as we can't use stock anyway, the way Drupal works ...
And for ESI and Varnish caching, etc. to work properly there are other things we need to take care of first, too - for example fix form and route CSRF handling.
It is OSS software, so if this will be better and cool and allow some really nice things that we really need, for all means lets implement it and let the implementation prove why its so more cooler for itself, but just for the sake of using it, I would say this needs to show why it is so much better first.
- So using a persistent page cache on disk? Yes, we can do that.
- Using HttpCache out of the box? We probably do not gain much at this point anymore.
Leaving as postponed for now.
I will create the meta for the authenticated user page cache as we are sooo close to having that working - or at least empowering contrib to do so and do it in 8.1 or such.
Comment #103
znerol CreditAttribution: znerol commentedThe next step in #2257695: [meta] Modernize the page cache is #2348679: Move the remaining procedural page cache code to the page cache stack middleware which will pack remaining scattered bits of internal page cache into a swappable middleware.
He, the very reason I actually started working on the now superseeded #2177461: Refactor page caching into a service issue was that I realized that a D8 port of Authcache will be much easier when core gets more flexible in this regard.
Comment #104
Crell CreditAttribution: Crell commentedI am sitting with Fabien at Symfony Live NYC right now and asked about the eval(). That's part of the ESI handling in HttpCache. In a nutshell, ESI tags in the cached page are converted to small PHP snippets that, when the cached page is loaded, will call back to the inner Kernel to make the esi request.
See Esi::handleEsiIncludeTag() for the code that creates the PHP.
Comment #105
chx CreditAttribution: chx commentedSo there's some capability to set ESI tags and then ... you resolve them request time by making more Drupal requests? That may be useful for authcache but for normal cache I would rather not sure anything like that...
Comment #106
Fabianx CreditAttribution: Fabianx commentedWe do support something like that via #post_render_cache already now in Core (without eval) and we need to store additional information anyway like cache tags, so as already stated this Component is not useful at the moment.
But thanks for the information!
Comment #107
Crell CreditAttribution: Crell commentedAny in-process ESI emulation is going to have to do something like that one way or another, as you will need to make a new request to get the content to replace. I didn't ask how deeply it supports force clearing via tags, which I agree would be a problem.
Comment #108
Wim LeersSymfony 2.6.6 was just released. With a security hole in their ESI support due to that
eval()
.See http://symfony.com/blog/symfony-2-6-6-released, and the fix at https://github.com/symfony/symfony/commit/195c57e1f50765aff33137689b16e1... ("Safe escaping of fragments for eval()").
The code remains equally undocumented and has become only less comprehensible.
Comment #109
Fabianx CreditAttribution: Fabianx commentedchx++ for saying 'no'
Also we are close to have #102 working in core now.
Comment #110
znerol CreditAttribution: znerol commentedhttp://symfony.com/blog/cve-2015-2308-esi-code-injection
Comment #111
dawehnerSounds at least like a 8.1 work if not 9.x or never
Comment #112
Wim Leers+1
Comment #128
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis has been PNMI for 7+ years. Think it can be closed out but if still a valid task in 11.x we should move to Active or NW with an updated issue summary. Till then closing out though.