Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- See #2469431-100: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts and related comments.
- Note that this only makes sense when Page Cache is off. So needs a
hook_requirements()
etc also.
Proposed resolution
- Primary condition, i.e. to use BigPipe placeholders at all: there must be an open session. (Compared to HEAD's requirement of the user needing to be authenticated.)
- Secondary condition, i.e. to use BigPipe JS placeholders:
$_SESSION['big_pipe_has_js']
. This is currently only set upon logging in. #2 contains the proposed approach to make that work even for anonymous users. - Finally, if sensible to do here, implement #2 in such a way that the WTF during the installation process documented in the README disappears. Quoting the README:
Note: None of the existing sessions will be able to use BigPipe. Log out and log in to benefit. To fix that, see https://www.drupal.org/node/2603046.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#29 | bigpipe_anon-2603046-29.patch | 15.98 KB | Wim Leers |
Comments
Comment #2
Wim LeersSo, this issue will have to update this part of
BigPipeStrategy
:We should simply remove that.
The reason we do that is twofold:
The solution for those two problems: #2469431-100: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts:
i.e.set a "no-JS" cookie without using JS, but instead using a redirect, which only affects no-JS users. This means the first page load will be slower for no-JS users (because of the redirect), but subsequent page loads will be faster.
Comment #3
Wim Leers@yched reported a duplicate at #2619092: Get rid of $_SESSION['big_pipe_has_js'] ? :)
Comment #4
andypostAlso a line in README would be helpful about how to use the module with 2 core's caching modules
Comment #5
Wim Leers#4: very good point! Opened a separate issue for that #2621394: Initial README + document relation to Page Cache & Dynamic Page Cache modules — let's continue the documentation aspect there :)
Comment #6
Wim LeersBTW, one thing to take into account here is that the vast majority of Drupal sites would actually have worse performance when they'd use BigPipe for anonymous users!
BigPipe is designed for making dynamic responses faster. But for anonymous users, the vast majority of Drupal sites don't have anything dynamic! (The biggest exception is probably e-commerce sites, but other than that, it really depends on the specific site.)
Drupal 8 has gotten very far with automatically detecting which things are too dynamic and should therefore not be cached. But… the "messages" block is present on every page, and always has a placeholder (for rendering messages). So, if we would say "we only enable BigPipe responses for anonymous users if there are >=1 placeholders on the page"… then we'd actually still apply BigPipe 100% of the time.
OTOH, status messages can only appear if there is a session. So perhaps we can use not "placeholders or not?" as the trigger, but "session or not?", that seems like it would be okay.
Comment #7
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedActually #6 is brilliant!
Session => BigPipe it.
No Session => Let anon page_cache deal with it.
That also means we can store the NoJS available information in the SESSION - would just mean that users removing product from cart would continue to have a SESSION - no big deal and could even auto-detect that (no other session data) and remove the cookie again.
So great!
Comment #8
Wim LeersCool :)
That approach allows us to specify the cache context
'cookie:' . session_name()
. That is condition #1 (having a session at all). Condition #2 will be the value in the session that indicates whether JS is enabled or not (cache context:session
).So then I think the IS can be updated to reflect the 3 things that need to happen here:
$_SESSION['big_pipe_has_js']
. This is currently only set upon logging in. #2 contains the proposed approach to make that work even for anonymous users.Comment #9
Wim LeersThis patch implements #6.
Comment #10
Wim LeersFix the cache context specified for BigPipe placeholders as explained in #8.
Comment #11
Wim LeersI looked into #8.3, and I think that should be a separate issue. We can do make it work thanks to
hook_install()
. But! Simply deleting all existing sessions seems dangerous/problematic because thenhook_user_logout()
doesn't get invoked (on the other hand, we could demand that such modules make the necessary changes usinghook_modules_installed()
). Some sites may depend on that. Invoking that hook for all existing sessions, which may very well be 1 million sessions… that doesn't seem like it would scale :PBUT! :)
Implementing #2 solved it too! Because the BigPipe module now assumes JavaScript, there is no need to log in again (through a specific form even): BigPipe using JS placeholders is now done automatically. But thanks to the technique outlined in #2, we can rely on the browser redirecting — only when JS is disabled — to a URL that responds with a
big_pipe_nojs
cookie and a redirect back to the original location. From every request with that cookie, we use BigPipe no-JS placeholders instead of BigPipe JS placeholders. Et voilà: it works completely transparently!Leaving up for review/RTBC by @Fabianx, but IMO this patch is ready.
Once this is committed, I propose we tag beta2, because this will take away the last hurdle/WTF when enabling BigPipe on Drupal 8.
Comment #12
yched CreditAttribution: yched commentedYay for $_SESSION['big_pipe_has_js'] killed dead !
I won't be the one RTBC'ing this, that job really is for @Fabianx :-), just a couple questions / nitpicks :
We really overwrite the whole $page['#attached'] ? What if other implementations of hook_page_attachments() ran before and added stuff there ?
(also, that is some nesting :-))
Is that still accurate ? The current code unconditionally sets the cookie - ok, unless the user has no reason to be visiting that route to begin with (he has no session and is not eligible for BigPipe anyway, or he already has the cookie).
For clarity of the overall JS/noJS flow, it could be more worthwhile for this short description to say "set the cookie *and redirect to the original page*", and leave the sanity checks out of the explanation ?
(also, shouldn't the code also bail out if there is no 'destination' query param ?)
Can't a controller get the Request just by putting it in the method params ?
+ Nitpick : the method name is a bit misleading, this doesn't really check whether JS is off - if we get there, we already know it's off :-)
--> maybe setNoJsCookie() ?
Comment #13
Wim Leers#12: Excellent review, thank you, @yched!
But,
would be a good addition indeed! HTTP 500 seems appropriate in that case.RE: method name: I agree, I didn't quite like the name yet either. Your suggestions is excellent. Will do :)
@Fabianx: feel free to still RTBC, I'll in any case apply the above suggestions :)
Comment #14
Wim LeersAddressed all feedback from #12, as described in #13.
Comment #15
Wim LeersAnd actually, looking at #14, it now clearly makes no sense anymore to favor DIC over specifying
Request $request
as a parameter. So, changed to that. Fewer LoC!Comment #16
Wim LeersFound just one more nitpick after a self-review.
Comment #17
yched CreditAttribution: yched commentedGreat simplification :-)
#12.2: and
Yeah, what I meant was that this "if there is a session" part, while true (the controller does check that), isn't really relevant to the understanding the actual flow, since if you don't have a session you're not sent to that controller to begin with, the actual decision on "is there a session ?" happens earlier, in big_pipe_page_attachments() of the "original page".
The checks that the controller makes ("is there a session", but also "is the cookie already there", "is there a destination") are not where the flow is implemented, they are just additional safeguards if someone artificially and intentionnally visits that controller without being sent there by big_pipe_page_attachments(), but they could be removed without breaking the regular flow, right ?
The important info that is missing, OTOH, is that after setting the cookie we do redirect back to the original page :-)
Thus, having that short description text say "set the cookie and redirect back" would be more helpful than "set the cookie if there is a session" IMO.
The safeguard checks ("bail out if not sent there by the big_pipe_page_attachments() redirect") are more incidental, and seem more like they deserve an inline code comment ?
Painful pedant shuts up now :-)
Comment #18
Wim LeersYou're the best pedant in that case :) ;)
You're absolutely right. Will fix.
Comment #19
Wim LeersComment #20
Wim LeersTurned the
big_pipe_nojs
cookie into a constant. Should help with maintenance.Comment #21
yched CreditAttribution: yched commented@Wim++ :-)
Comment #22
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedGreat patch!
I have some concerns, but don't have time to review in detail, so high level:
- I feel uncomfortable directly dealing with cookies: . session_name() - that feels like an implementation detail to me. Proposal (and we can ship that context until we move it to core in 8.1):
Use a context like session.has:1 or session.has_session or something like is, like user.is_anonymous (or however we called that).
Elaboration: If the session comes from something else - we don't want to put this to cookies only.
- The same we should check / call the function whatever determines if we have a SESSION and not look into cookies.
I think a good example is in the Default Request policies using the Session Storage to determine if a session is active.
- The cookies:big_pipe_no_js however is great and shows the flexibility of the contexts.
- IMHO the cache contexts should be added where the placeholders are created.
---
Nits:
- doProcessPlaceholders feels excessive to add - I don't see the value with the early return.
-
big_pipe_no_js should be a constant in the class - no magic names, please.---
Future possibilities (train of thought, not actionable):
Could think about removing the cookie on any POST that does no longer have a SESSION (e.g. user cleared cart), but given most Varnish configurations strip all cookies anyway, except SESSION to determine anonymous, but we only use this when we have a SESSION we are in sync with the standard implementation, so this should be fine.
Comment #23
Wim LeersIt's an implementation detail. But it's also the reality for 99.9% of sites. So, I think it's fine to do this, since it's pragmatic.
session.has
doesn't work because that notation means it's a value stored in the session. Which is not true. We need to know whether there is a session at all, not a specific value within a session, nor which particular session. Just whether there is a session. And that's what'cookies:' . session_name()
captures.What we can do in the future, is add some API to Drupal core that gives us the cache context to determine whether there is a session or not. By default, it would return
'cookies:' . session_name()
. But if a module is installed that supports different kinds of sessions, it'd return the appropriate cache context in that scenario. But solving that here is premature IMO.user.is_anonymous
definitely does not work, because it means BigPipe once again cannot work for anonymous users.Note that
BigPipeStrategy
already does this. So actually, it's this service that should return a cache context IMO. And I think that's actually perfectly doable: let BigPipe extendSessionConfigurationInterface
, subclassSessionConfiguration
and override/decorate the service with our subclass. Then move that into Drupal 8.1 core. This feels not premature (unlike what I said above), because we add just a single method, with one bit of metadata, to an existing abstraction. Thoughts?:)
Let me answer by quoting code:
That refers to this part, right? Specifically, you disagree with the deletions at the bottom, right?
The thing is that the cache contexts do depend on how it was determined that either the no-JS or JS placeholder should be used.
I could hardcode the
'cookies:' . session_name()
part again and only add the'cookies:' . static::NOJS_COOKIE
context in the top-level else. But IMO that is misleading, precisely because not the::createBigPipe(No)JsPlaceholder()
can know which cache contexts are appropriate, only the caller can know that.I've documented the reason:
i.e. this is purely code organization that also buys flexibility. It makes it very easy to see that we process placeholders unless reason X. That makes it easy for subclasses to add reasons Y, Z, etc.
Oohhh, yes, that would be nice. Anon users, cart emptied, so nothing personalized anymore, so back to getting responses from Page Cache. That'd be lovely.
Comment #24
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented#23:
- I don't think cookies:session_name() does achieve that, it should just give back the same as the SessionCacheContext, because of:
vs.
which also points to a bug in Core as getSession() can return NULL (http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Request.html...).
So while user.roles:anonymous indeed would be .is("anoynmous"), the same is not true for cookies where we actually divide the cache on the value.
- I don't see why we can't have children of the contexts with "functions" - even if those are hard-coded for speed reasons.
cookies.has("session_name") == TRUE / FALSE and cookies.has == any cookie exists
is still reducible to cookies:session_name and cookies, so not breaking the hierarchy.
- An easier way obviously is to have session.exists as cache context.
- Not getting why we would need to Subclass session storage.
Comment #25
andypostSession API is not finished yet, so better to relay on current request now
Comment #26
BerdirNote from an IRC discussion I just had with Wim:
Would be good to consider supporting all kinds of cookies and somehow allow to configure it. We have personalization/regionalization based on a custom cookie to avoid disabling the page cache completely for anon users.
Comment #27
andypostGreat idea! just dispatch proper event
Comment #28
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented#25 - #27:
I don't think big_pipe needs to support anything here.
To explain high-level:
- We want to use BigPipe whenever it is certain that this page would not land in the internal page_cache or be cached by Varnish.
In all cases where the page can be cached it is quicker to not use BigPipe, but to instead populate the page cache / Varnish with a static page.
- There is a request policy that checks for a session in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21PageCache...
In theory BigPipe should also be active for all other cases where either internal page cache or external page cache don't cache the page, but that can be follow-up to make this more flexible. (and yes here an alter hook or event could be added in a follow-up - if that is what you mean)
The cookie part of this is completely different as it just is a variation of the page based on a cookie's value.
It gets tricky when talking about caching though, because hook_page_attachments is cached in DPC.
Proposal to resolve
Lets skip all cache variation and just add a placeholder in hook_page_attachments unconditionally, then replace it after DPC with either the noscript tag or nothing - depending on SESSION state and if we BigPipe at all.
Then we also don't need any new cache contexts - as we are purely after caching and have made sure neither internal (as we only deliver with SESSION) nor external (as we send the appropriate max-age headers) proxies cache it.
Thoughts?
Train of thought (Follow-up)
Overall I think long-term we need our own page_cache response policy so that BigPipe can turn off internal page_cache for more complex cases. That would be more clean anyway.
e.g. the use case is a complex page that should never be cached - even for anonymous (regardless of SESSION), but could safely be BigPipe'd.
Comment #29
Wim Leers#24:
It does.
cookies:FOO
returns the value of the FOO cookie if it exists, and returns the empty string otherwise. Whereassession
indicates the session ID, which isPerhaps, but it never does in Drupal 8. I investigated this.
\Drupal\Core\StackMiddleware\Session::handle()
starts a session for every request:But note that
\Drupal\Core\Session\SessionManager::start()
does this:Hence
SessionCacheContext
works always. Counterintuitive and messy for sure, but all of the session/authentication handling is, see #2371629: [meta] Finalize Session and User Authentication API for improving that.This also shows why we cannot use the
'session'
cache context to detect the presence/absence of a session, but we can use'cookies:' . session_name()
."functions" in cache contexts would be an entirely new concept.
Hm…… AFAICT this would indeed be possible! That would then just do
return $this->sessionConfiguration->hasSession($request)
and that'd be it. What I wrote in #23 about extendingSessionConfigurationInterface
would not be necessary then. IOW: I think what I wrote in #23.2 was wrong. Becausesession.exists
relates tosession
asuser.is_super_user
relates touser
.I'm +1 for this approach I think. But it really belongs in core, not in this module. Though we can of course first put it in the BigPipe module and then propose it for Drupal core inclusion. Thoughts?
#25: Thanks for that link, I posted a comment there at #2371629-35: [meta] Finalize Session and User Authentication API, to capture the relevant bits of #2603046-23: Support anonymous users.
#26: thanks for commenting. For a bit more context, here's the relevant part of our IRC conversation:
#27: Symfony events are expensive.
#28:
Agreed. That's also what I wrote in #6 :)
In fact, that's also what I told Berdir in IRC. This IRC snippet follows the above IRC snippet:
So, I think we are all (Fabianx, Berdir, I) on the same page regarding that.
This part of #28 further confirms that:
Which means nothing else is needed for Berdir's use case, all that is needed is the
'session.exists'
cache context mentioned above, in my reply to #24.What is tricky about this?
We should do this only if it's better. i.e. what is best for most sites: to have up to two Dynamic Page Cache entries for every page (i.e. with/without BigPipe no-JS cookie, i.e. with/without the redirect encapsulated in a
<noscript>
tag), or replace this placeholder on every page load? Considering that this is a<meta>
tag, and those tags always are listed before CSS and (header) JS assets, that means we're blocking the loading of CSS/JS on PHP replacing that placeholder.Furthermore, it would be a special case placeholder: if we'd treat it like any other placeholder, it would itself be converted into a BigPipe JS placeholder… which of course defeats the purpose. So we'd have to special-case it, to force it to be a BigPipe no-js placeholder. Which we totally could do, but it's not clear if that's actually desirable.
Thoughts?
We could then (if we'd placeholder the
<noscript>
meta tag, see above) indeed live without a new cache context, but…BigPipeStrategy
still needs it to correctly communicate the cacheability of its transformed placeholders. We could in theory live without that, but it's better to specify the correct cacheability metadata. Especially when other placeholder strategies are added to the mix.Whew, that was a lot to address. So close!
Comment #30
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented#29: When I meant placeholder, I meant placeholder not as in a normal placeholder, but more in a fixed constant which just gives us a place to replace things before sending the response.
cookies: . session_name() will still create a new bucket for all authenticated users, which is still sub-optimal. :-/
Comment #31
Wim LeersOkay.
But to this:
my answer is still:
You didn't reply to that part of #29. Thoughts about that?
Comment #32
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented#30 / #31 have been just a mis-understanding.
This is a reply to #29:
- session.exists is indeed the most elegant solution and it belongs obviously in core (8.[01].x branch based on committer discretion).
- cookies: . session_name() would still lead to per-session, per-route cache in DPC and hence is not a valid solution.
While the 1% case still is valid, too that BigPipe wants to be used on a Page without a SESSION, this is an edge case and does not prevent this from going in.
Patch looks great and again shows that the simplicity of creating new contexts is a full win.
RTBC
Comment #33
Wim LeersFully agreed with #32.
Yay!
I can't give @nod_ issue credit, but I can give him commit credit.