Problem/Motivation

Proposed resolution

  1. 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.)
  2. 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.
  3. 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

So, this issue will have to update this part of BigPipeStrategy:

  public function processPlaceholders(array $placeholders) {
    // @todo Move to a ResponsePolicy instead.
    // @todo Add user.roles:authenticated cache context.
    if (!$this->currentUser->isAuthenticated()) {
      return [];
    }

We should simply remove that.

The reason we do that is twofold:

  1. Anon users don't have sessions by default, so we can't see whether that user's session has JS enabled or not.
  2. We run the "does the user have JS?"-check on log in.

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:

Solution:

- Assume JavaScript is on
- Add <meta http-equiv refresh=""> inside <noscript> tags inside the html_head to forward to /bigpipe/js-off?destination=<orig url>
- That user is then blacklisted for the remainder of their session and apart from 2 redirects, it is 100% transparent and gracefully degrades the UX

While that could still be made generic, many other use cases can probably use tags in a different way.

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.

Wim Leers’s picture

Version: » 8.x-1.0-beta1
Status: Postponed » Active
Related issues: +#2619092: Get rid of $_SESSION['big_pipe_has_js'] ?

@yched reported a duplicate at #2619092: Get rid of $_SESSION['big_pipe_has_js'] ? :)

andypost’s picture

Also a line in README would be helpful about how to use the module with 2 core's caching modules

Wim Leers’s picture

#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 :)

Wim Leers’s picture

BTW, 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.

Fabianx’s picture

Actually #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!

Wim Leers’s picture

Version: 8.x-1.0-beta1 » 8.x-1.x-dev
Issue summary: View changes

Cool :)

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:

  1. 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.)
  2. 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.
  3. 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.
    
Wim Leers’s picture

Status: Active » Needs review
FileSize
4.98 KB

This patch implements #6.

Wim Leers’s picture

FileSize
6.67 KB
2.39 KB

Fix the cache context specified for BigPipe placeholders as explained in #8.

Wim Leers’s picture

Assigned: Unassigned » Fabianx
FileSize
10.34 KB
9.16 KB

I 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 then hook_user_logout() doesn't get invoked (on the other hand, we could demand that such modules make the necessary changes using hook_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 :P

BUT! :)

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.

yched’s picture

Yay 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 :

  1. +++ b/big_pipe.module
    @@ -2,57 +2,41 @@
    +    $page['#attached'] = [
    +      'html_head' => [
    +        [
    +          [
    +            // Redirect through a 'Refresh' meta tag if JavaScript is disabled.
    +            '#tag' => 'meta',
    +            '#noscript' => TRUE,
    +            '#attributes' => [
    +              'http-equiv' => 'Refresh',
    +              'content' => '0; URL=' . Url::fromRoute('big_pipe.nojs', [], ['query' => \Drupal::service('redirect.destination')->getAsArray()])->toString(),
    +            ],
    +          ],
    +          'big_pipe_detect_nojs',
    +        ],
    +      ],
    +    ];
    

    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 :-))

  2. +++ b/src/Controller/BigPipeController.php
    @@ -0,0 +1,89 @@
    +   * Sets a 'big_pipe_nojs' cookie if there is a session.
    

    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 ?)

  3. +++ b/src/Controller/BigPipeController.php
    @@ -0,0 +1,89 @@
    +  public function checkNoJs() {
    +    $request = $this->requestStack->getCurrentRequest();
    

    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() ?

Wim Leers’s picture

#12: Excellent review, thank you, @yched!

  1. OOPS! Great catch! I'll fix that :)
  2. Yes, that comment is accurate — see the latter condition in the if-test:
    +++ b/src/Controller/BigPipeController.php
    @@ -0,0 +1,89 @@
    +    if ($request->cookies->has('big_pipe_nojs') || $request->getSession() === NULL) {
    +      throw new AccessDeniedHttpException();
    

    But, also, shouldn't the code also bail out if there is no 'destination' query param would be a good addition indeed! HTTP 500 seems appropriate in that case.

  3. RE: parameter vs DIC: Yes, and I first had it like that. But I figured I should go with the injected variant instead. Not sure which actually is considered better.
    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 :)

Wim Leers’s picture

FileSize
14.07 KB
3.89 KB

Addressed all feedback from #12, as described in #13.

Wim Leers’s picture

FileSize
12.92 KB
3.74 KB

And 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!

Wim Leers’s picture

FileSize
12.92 KB
847 bytes

Found just one more nitpick after a self-review.

yched’s picture

Great simplification :-)

#12.2: and

+++ b/src/Controller/BigPipeController.php
@@ -0,0 +1,59 @@
+  /**
+   * Sets a 'big_pipe_nojs' cookie if there is a session.
...
+  public function setNoJsCookie(Request $request) {

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 :-)

Wim Leers’s picture

You're the best pedant in that case :) ;)

You're absolutely right. Will fix.

Wim Leers’s picture

FileSize
13.26 KB
2.3 KB
Wim Leers’s picture

FileSize
13.46 KB
5.55 KB

Turned the big_pipe_nojs cookie into a constant. Should help with maintenance.

yched’s picture

@Wim++ :-)

Fabianx’s picture

Status: Needs review » Needs work

Great 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.

Wim Leers’s picture

Status: Needs work » Needs review

I feel uncomfortable directly dealing with cookies: . session_name() - that feels like an implementation detail to me.

It'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.

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.

  1. This means a layer of indirection for 0.01% of sites. I don't think that's worth it. At least not now. IMHO we can add it in the future, if/when the need arises.
  2. 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.
  3. user.is_anonymous definitely does not work, because it means BigPipe once again cannot work for anonymous users.

- 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.

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 extend SessionConfigurationInterface, subclass SessionConfiguration 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?

- 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.

Let me answer by quoting code:

+++ b/src/Render/Placeholder/BigPipeStrategy.php
@@ -52,31 +64,61 @@ use Drupal\Core\Session\AccountInterface;
+    $non_html_placeholder_cache_contexts = ['cookies:' . session_name()];
+    $html_placeholder_cache_contexts = ['cookies:' . session_name(), 'cookies:' . static::NOJS_COOKIE];
+
     $overridden_placeholders = [];
     foreach ($placeholders as $placeholder => $placeholder_elements) {
       // BigPipe uses JavaScript and the DOM to find the placeholder to replace.
@@ -94,16 +136,18 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {

@@ -94,16 +136,18 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {
       // @see \Drupal\Core\Form\FormBuilder::renderPlaceholderFormAction()
       if ($placeholder[0] !== '<' || $placeholder !== Html::normalize($placeholder)) {
         $overridden_placeholders[$placeholder] = static::createBigPipeNoJsPlaceholder($placeholder, $placeholder_elements);
+        $overridden_placeholders[$placeholder]['#cache']['contexts'] = $non_html_placeholder_cache_contexts;
       }
       else {
-        // If the current session doesn't have JavaScript, fall back to no-JS
-        // BigPipe.
-        if (empty($_SESSION['big_pipe_has_js'])) {
+        // If the current request/session doesn't have JavaScript, fall back to
+        // no-JS BigPipe.
+        if ($this->requestStack->getCurrentRequest()->cookies->has(static::NOJS_COOKIE)) {
           $overridden_placeholders[$placeholder] = static::createBigPipeNoJsPlaceholder($placeholder, $placeholder_elements);
         }
         else {
           $overridden_placeholders[$placeholder] = static::createBigPipeJsPlaceholder($placeholder, $placeholder_elements);
         }
+        $overridden_placeholders[$placeholder]['#cache']['contexts'] = $html_placeholder_cache_contexts;
       }
     }
 
@@ -141,7 +185,6 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {

@@ -141,7 +185,6 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {
     return [
       '#markup' => '<div data-big-pipe-selector="' . Html::escape($big_pipe_js_selector) . '"></div>',
       '#cache' => [
-        'contexts' => ['session'],
         'max-age' => 0,
       ],
       '#attached' => [
@@ -178,7 +221,6 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {

@@ -178,7 +221,6 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {
     return [
       '#markup' => '<div data-big-pipe-selector-nojs="' . $html_placeholder . '"></div>',
       '#cache' => [
-        'contexts' => ['session'],
         'max-age' => 0,
       ],
       '#attached' => [
 

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.

Nits:

- doProcessPlaceholders feels excessive to add - I don't see the value with the early return.

I've documented the reason:

 * (This is the default, and other modules can subclass this placeholder
 * strategy to have different rules for enabling BigPipe.)

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.

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.

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.

Fabianx’s picture

Status: Needs review » Needs work

#23:

- I don't think cookies:session_name() does achieve that, it should just give back the same as the SessionCacheContext, because of:

$this->requestStack->getCurrentRequest()->cookies->get($cookie);

vs.

$this->requestStack->getCurrentRequest()->getSession()->getId();

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.

andypost’s picture

Session API is not finished yet, so better to relay on current request now

Berdir’s picture

Note 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.

andypost’s picture

Great idea! just dispatch proper event

Fabianx’s picture

#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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.98 KB
6.8 KB

#24:

I don't think cookies:session_name() does achieve that,

It does. cookies:FOO returns the value of the FOO cookie if it exists, and returns the empty string otherwise. Whereas session indicates the session ID, which is

which also points to a bug in Core as getSession() can return NULL

Perhaps, but it never does in Drupal 8. I investigated this. \Drupal\Core\StackMiddleware\Session::handle() starts a session for every request:

public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
    if ($type === self::MASTER_REQUEST && PHP_SAPI !== 'cli') {
      $session = $this->container->get($this->sessionServiceName);
      $session->start();
      $request->setSession($session);
    }

But note that \Drupal\Core\Session\SessionManager::start() does this:

      // Randomly generate a session identifier for this request. This is
      // necessary because \Drupal\user\SharedTempStoreFactory::get() wants to
      // know the future session ID of a lazily started session in advance.
      $this->setId(Crypt::randomBytesBase64());

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().

- I don't see why we can't have children of the contexts with "functions" - even if those are hard-coded for speed reasons.

"functions" in cache contexts would be an entirely new concept.

- An easier way obviously is to have session.exists as cache context.

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 extending SessionConfigurationInterface would not be necessary then. IOW: I think what I wrote in #23.2 was wrong. Because session.exists relates to session as user.is_super_user relates to user.

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:

14:56:22 <WimLeers> berdir: the only assumption is that anon users must have sessions to get personalization.
15:04:08 <berdir> WimLeers: one input on the session thing, we have a regionalization concept where a user can pick a city and gets local news. we don't use a session but a custom cookie for that, mostly because we still want to be able to return most pages like articles from page cache. so would be interesting to support not just the default session cookie but others too
15:04:32 <WimLeers> berdir: okay, that is SUPER valuable.
15:05:19 <WimLeers> berdir: so you have a use case where you want personalization not just based on "session cookie? yes/no", but based on "region cookie? yes/no".
15:05:48 <WimLeers> berdir: that goes actually directly to Fabianx's concern that the session may not be cookie-based
15:05:56 <WimLeers> berdir: this is similar to that, but different
15:06:04 <WimLeers> berdir: different in that you don't care about session here, but about *something else*
15:06:36 <berdir> WimLeers: exactly, basically one or more blocks are then different on the frontpage and show different things

#27: Symfony events are expensive.


#28:

- 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.

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:

15:06:34 <WimLeers> berdir: although I would think BigPipe is less of a great fit for that; it seems a Page Cache that is aware of this cookie would be better for you
15:07:01 <berdir> WimLeers: right now it works in the combination of disabling page cache and then fastly varies by that cookie
15:07:25 <WimLeers> berdir: But there's no reason you couldn't have a "per-region PageCache", righit?
15:07:39 <WimLeers> berdir: that seems more efficient, no?
15:14:23 <Fabianx-screen> I agree
15:14:44 <Fabianx-screen> Should just extend the page cache to customize the CID for that case.
15:15:03 <Fabianx-screen> I still think there is a use case for an advanced page cache that is aware of cache contexts, as here its just cookies:name.
15:15:09 <Fabianx-screen> but that can be contrib.
15:21:51 <berdir> yeah, we were thinking about that, that's also why I was interested in making the cid alterable/customizable. But this was all developed/designed long before page cache was a module/service where it was way hard to customize it
15:21:54 <berdir> different story now
15:23:21 <berdir> one argument was that the output only varies on some pages and I thought it is more efficient to not cache the page in drupal (we still have fastly that varies by it) than caching every page for every value of that cookie (could be like 50 variations or something). but of course we can also implement the cid dynamically so it just has that cookie when needed
15:24:21 <berdir> to not cache the page => to not cache a few specific pages

So, I think we are all (Fabianx, Berdir, I) on the same page regarding that.

This part of #28 further confirms that:

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)

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.

It gets tricky when talking about caching though, because hook_page_attachments is cached in DPC.

What is tricky about this?

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.

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?

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.

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!

Fabianx’s picture

#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. :-/

Wim Leers’s picture

When I meant placeholder, I meant placeholder not as in a normal placeholder, but more in a fixed constant

Okay.

But to this:

cookies: . session_name() will still create a new bucket for all authenticated users, which is still sub-optimal. :-/

my answer is still:

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.

You didn't reply to that part of #29. Thoughts about that?

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

#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

Wim Leers’s picture

Assigned: Fabianx » Unassigned
Status: Reviewed & tested by the community » Fixed

Fully agreed with #32.

Yay!

I can't give @nod_ issue credit, but I can give him commit credit.

  • Wim Leers committed a699c42 on 8.x-1.x
    Issue #2603046 by Wim Leers, Fabianx, nod_, yched: Support anonymous...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.