Render site alerts as part of the page content

Created on 17 June 2022, over 2 years ago
Updated 9 September 2024, 3 months ago

First of all, big fan of the work done on the module so far, especially the features introduced in 2.x!
I just thought it was worth documenting a concern I had regarding performance/caching.

Problem/Motivation

Site alert and page filtering is currently handled on the frontend, however this has certain functionality/performance implications:

  1. Special internal routes and aliases such as /node/2, <front> aren't supported. Routing conditions should ideally be handled in the backend
  2. The AJAX callback returns site alert for every single page (including pages which aren't relevant to the user/current page), which isn't very scalable if there is a lot of site alerts
  3. An AJAX request is made on every page request, which shouldn't be necessary if appropriate cache tags/contexts are set on the initial site alert render added to the initial page content, should significantly reduce server overhead especially when site alerts are only required on specific pages.

Nice to have: Use Drupal.ajax() API rather than fetch() so that site AJAX behaviour is consistent. e.g. If the site requires non-standard authentication for backend requests.

Proposed resolution

Only site alerts relevant to the current route/page should be rendered and attached as part of the page just like a normal block would.

If full page caching is a concern, it can be implemented through the #lazy_builder API.

Whilst the page filtering can leverage Drupal's RequestPath \Drupal::service('plugin.manager.condition') Condition plugin.

And when processing site alert updates via AJAX in the controller, we can simply add the requested path to the request stack and let the Condition plugin do its work using logic along the lines of:

$requestStack = \Drupal::service('request_stack');
// Push the referring url onto the stack so that it can be filtered
// against. Keeping all the current request's cookies and sessions.
// Coming from "Drupal.url(drupalSettings.path.currentPath)"
$realPath = $current_request->request->get('currentPath', NULL);
if ($realPath) {
  $request = $current_request->duplicate([], [], [], NULL, [],
    [
      'REQUEST_METHOD' => 'GET',
      'REQUEST_URI' => $realPath,
    ] + $current_request->server->all());
  $requestStack->push($request);
}
// Renderer will handle filtering of site alerts using the Condition Plugin API.
$build = \Drupal::service('sitewide_alert.sitewide_alert_renderer')->build(
  // Coming from "drupalSettings.path.currentPathIsAdmin" (unsure if necessary)
  $current_request->request->get('currentPathIsAdmin', FALSE)
);
// Rest of the code ...
if ($realPath) {
  // Pop the pseudo request.
  $requestStack->pop();
}

Remaining tasks

Provide issue fork/patch.

User interface changes

Not visible to the user, but site alerts will now be stored as part of the page response (but hidden by default with a special class/style attribute), then the javascript will run through it to determine if it should be shown or not.

API changes

TBD

✨ Feature request
Status

RTBC

Version

2.0

Component

Code

Created by

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

Merge Requests

Comments & Activities

Not all content is available!

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

  • πŸ‡ΊπŸ‡ΈUnited States Greg Boggs Portland Oregon

    I would like to add to this to mention that on my Drupal 9 site, this module accounts for between 20%-50% of all the resources uses by a page load depending on the cache status of the page when loaded. While it's cool that it's fancy for folks who need that up to the second detail, we really just use it to display a note at the top of the page. So, spending 50% of our used server resources displaying a note at the top of the screen seems... like something we want to improve.

  • πŸ‡¬πŸ‡§United Kingdom malcomio

    Regarding the cache lifetime, that has been made configurable in ✨ sitewide_alert cache max age value is hard coded Fixed , but I agree that this module shouldn't be making so many requests, and ideally would include everything it needs in one page load.

  • πŸ‡ΊπŸ‡ΈUnited States chrissnyder Maryland

    I like this idea. I am wondering if we can implement it in a way that allows the alert rendering method to be configurable. For some projects I have worked on, the advantage of this module over other similar modules or a solution using normal blocks/views is that this module was designed to avoid breaking fully cached pages, including those cached downstream (varnish, reverse proxy), at a time when a new alert is published site wide. One of the main goals of loading the alerts as a separate request was to allow pages to be fully cached independently of active alerts. In addition, this module also supports showing alerts for those who have already loaded the page.

    Example situation when this is important:

    A university, school, or government agency needs to publish a very important time-sensitive alert sitewide at a time when traffic to the site is abnormally high. This may be an alert regarding public safety. With this change, adding a sitewide alert would cause the page cache of all the pages on the site to be invalidated (due to the cache tag), resulting in excessive stress on the site as it needs to rebuild all of the pages at a time when traffic is high and the information is important.

    However, this situation/use does not apply to all sites, so it should be configurable to allow the site builder to choose which loading method fits their project.

  • πŸ‡ΊπŸ‡ΈUnited States Greg Boggs Portland Oregon

    The module would literally crash any website using it while their traffic was abnormally high. So that feature does not exist despite what the project description says.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    8 pass
  • πŸ‡¬πŸ‡§United Kingdom malcomio

    Work in progress on a slightly different approach in https://git.drupalcode.org/project/sitewide_alert/-/merge_requests/41

    The change is fairly minimal - just moving the alerts into drupalSettings

    This probably needs further thought from a security and performance point of view.

    Also, as per #5, ideally we would make this configurable.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    8 pass
  • First commit to issue fork.
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Not currently mergeable.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    4 pass, 8 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    8 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    4 pass, 8 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    4 pass, 8 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    4 pass, 8 fail
  • πŸ‡ΊπŸ‡ΈUnited States mlncn Minneapolis, MN, USA

    Would absolutely love this with the further enhancement that if "Automatically Update (Refresh) Alerts" (polling) is disabled in /admin/config/sitewide_alerts and "Dismissible" is disabled for the alert(s), there is no reason to load the init.js (or any other) JavaScriptβ€” yet more optional performance enhancement and site admin flexibility for this excellent module.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    4 pass, 8 fail
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    4 pass, 8 fail
  • πŸ‡ΊπŸ‡ΈUnited States chrissnyder Maryland

    Would it be possile to include this feature as a configuration option so that the existing functionality is preserved for those who need it and the new ability to avoid the extra request is avoided for others?

  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Pipeline finished with Failed
    about 1 year ago
    Total: 821s
    #63568
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 261s
    #63592
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Pipeline finished with Success
    about 1 year ago
    Total: 798s
    #63601
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 121s
    #66146
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 93s
    #66147
  • Pipeline finished with Success
    about 1 year ago
    Total: 189s
    #66149
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    Waiting for branch to pass
  • Pipeline finished with Success
    about 1 year ago
    #66300
  • Status changed to Needs review 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States mlncn Minneapolis, MN, USA
  • Status changed to RTBC 10 months ago
  • πŸ‡­πŸ‡ΊHungary aron novak Hungary, Budapest

    We tested it on a client project, worked well.

  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update 9 months ago
    Waiting for branch to pass
  • Pipeline finished with Failed
    9 months ago
    Total: 159s
    #137477
  • First commit to issue fork.
  • Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update 7 months ago
    Waiting for branch to pass
  • πŸ‡ΊπŸ‡ΈUnited States rpayanm

    I added some suggestions and fixed the test error. Please review.

  • πŸ‡¨πŸ‡¦Canada ibullock London, ON

    +1 for RTBC, been using in the wild for a few months now without issue.

  • +1 for this. works as expected

  • πŸ‡―πŸ‡΄Jordan Qusai Taha Amman

    Create a patch from the MR

  • @qusai taha, you can just click on the plain diff and that's a patch already, no need for a separate patch

  • πŸ‡―πŸ‡΄Jordan Qusai Taha Amman

    Re-roll the patch to make it working with latest version

  • πŸ‡―πŸ‡΄Jordan Qusai Taha Amman

    Re-roll the patch to make it working with latest version

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave
Production build 0.71.5 2024