Don't set page-level user cache context; do not bust dynamic page cache

Created on 13 May 2018, over 6 years ago
Updated 20 May 2023, over 1 year ago

I ran into some issues with this module debugging some performance issues for a client; in particular, it turned out that Olark's use of hook_page_bottom(), which furthermore set the user cache context, effectively means dynamic page cache will return UNCACHEABLE with the default settings shipped with core. (The user cache context is considered high-variability and if it's not placeholdered, DPC will refuse to cache the page overall.)

Curiously, many modules including this one and the popular google_analytics inject user-specific data into drupalSettings in hook_page_attachments() (e.g., to provide a user ID) however this is also problematic from a caching perspective. See πŸ“Œ hook_page_(attachments|attachments_alter|top|bottom)() should specify the right cacheability metadata Needs work . I've searched some of their issue queues and this hasn't surfaced, or I didn't find a ticket. That's not particularly surprising, however, given that the current cache context setting effectively disabled DPC and the internal page_cache module doesn't work for authenticated users.

The attached patch refactors the non-personalized code for the Olark widget itself, and in order to avail ourselves of auto placeholdering that would be compatible with DPC provides a block that does vary by user and makes the appropriate additions to drupalSettings.olark. I would like to have found a more elegant solution than a block, however I think the way the render system works basically requires that approach. I did do some trial-and-error on setting #attached on some elements that we could reliably find in a standard site, but that seemed both a little hackish and fragile.

πŸ› Bug report
Status

Needs review

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡§πŸ‡ͺBelgium tim-diels Belgium πŸ‡§πŸ‡ͺ

    Cache is a big concern to implement 3rd party tools. Thanks for the work already done.
    I do have a few concerns about the implementation:

    • The code is always triggered, even when "enable admin" is unchecked. This was a small code error with moving code.
    • When fixing the above issue, I've refactored the code to have a helper class to define if Olark is enabled and allowed to run the code after. This is done in the hook_page_attachments, hook_page_bottom and the new block plugin. This will also allow to easier add functions/methods and use them in other parts of the code.
    • The block is not shown in the block add form as the annotation was wrong.

    The concerns are fixed in attached patch and an interdiff is provided to show the changes.

  • πŸ‡§πŸ‡ͺBelgium tim-diels Belgium πŸ‡§πŸ‡ͺ

    Ah and to answer the question in #4:

    They provide a block, but only for the placement of the code. This issue provides both and makes the other issue deprecated.
    You could close the other issue, but give them credit for their work here maybe?

  • πŸ‡§πŸ‡ͺBelgium tim-diels Belgium πŸ‡§πŸ‡ͺ

    Upgrade steps would need to be provided as the code now only places the script without personalization. So they would need to add the personalization block to have the personalization added to the DrupalSettings.

    Upgrade steps:

    • Add block to the page through the block overview
  • πŸ‡§πŸ‡ͺBelgium tim-diels Belgium πŸ‡§πŸ‡ͺ

    After some more testing it seemed the personalisation is not added correctly when only returning attached and cache.
    So needed to print an empty block to have the settings passed.

  • πŸ‡ΊπŸ‡ΈUnited States Chris Burge

    I'm not sure that this refactor (implementing with a block) is necessary. The reason that the 'user' cache context is being set for anonymous users is because the 'user' cache context is being set outside the auth user check in olark_page_bottom():

      // If the user is logged in, let's get some pertinent information and pass it
      // along as well.
      if ($user->id()) {     // <-- USER CHECK STARTS
        $user_page_url = Url::fromRoute('entity.user.canonical', ['user' => $user->id()], ['absolute' => TRUE])->toString();
        $js_settings['olark']['uid'] = $user->id();
        $js_settings['olark']['name'] = $user->getAccountName();
        $js_settings['olark']['mail'] = $user->getEmail();
        $js_settings['olark']['roles'] = t('roles @roles', ['@roles' => implode(', ', $user->getRoles(TRUE))]);
        $js_settings['olark']['userpage'] = $user_page_url;
        $js_settings['olark']['loggedinas'] = t('logged in as <a href=":url">@name</a>', [
          '@name' => $user->getAccountName(),
          ':url' => $user_page_url,
        ]);
      }     // <-- USER CHECK ENDS
      $page_bottom['olark'] = [
        '#markup' => Markup::create($settings->get('olark_code')),
        '#attached' => [
          'library' => ['olark/integration'],
          'drupalSettings' => $js_settings,
        ],
        '#cache' => [    // <-- CACHING METADATA IS ADDED TO ALL USERS
          'contexts' => ['user'],
          'tags' => ['user:' . $user->id()],
        ],
      ];
    
    
  • πŸ‡ΊπŸ‡ΈUnited States Chris Burge

    Attached patch is a different approach (hence no interdiff).

    Manual testing steps to verify customization continues to work:

    1. Log in as admin and navigate to front page.
    2. Open chat window and observe name and email fields are populated with admin user's info.
    3. Create a new user account.
    4. Login with user and navigate to front page.
    5. Open chat window and observe name and email fields are populated with the new user's info.
    6. Open a new browser window as the anonymous user.
    7. Open chat window and observe name and email fields are unpopulated.
Production build 0.71.5 2024