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

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

codebymikey created an issue. See original summary.

greg boggs’s picture

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.

Looks like the cache on the json page expires every 15 seconds even when the content does not change, so that's a place to start.

malcomio’s picture

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

chrissnyder’s picture

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.

greg boggs’s picture

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.

Essentially, the feature only works for moderate to low traffic sites, and in those cases, it's almost certainly way more efficient to expire tags smartly.

malcomio’s picture

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.

suryabhi made their first commit to this issue’s fork.

mlncn’s picture

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.

chrissnyder’s picture

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?

mlncn’s picture

Status: Active » Needs review
aron novak’s picture

Status: Needs review » Reviewed & tested by the community

We tested it on a client project, worked well.

rpayanm made their first commit to this issue’s fork.

rpayanm’s picture

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

ibullock’s picture

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

doxigo’s picture

+1 for this. works as expected

qusai taha’s picture

StatusFileSize
new61.89 KB

Create a patch from the MR

doxigo’s picture

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

qusai taha’s picture

StatusFileSize
new26.23 KB

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

qusai taha’s picture

StatusFileSize
new26.25 KB

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

smustgrave’s picture

Version: 2.x-dev » 3.0.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
qusai taha’s picture

StatusFileSize
new24.06 KB

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

qusai taha’s picture

StatusFileSize
new23.77 KB
smustgrave’s picture

Fixes should be in MRs not patches just fyi

edwardsay made their first commit to this issue’s fork.

edwardsay’s picture

Status: Needs work » Needs review
cyu’s picture

Status: Needs review » Needs work

Thanks for the 3.x patch. That's working well for me, but I'm getting some errors thrown from code in SitewideAlertsController.php.

$sitewideAlerts = $this->sitewideAlertManager->activeVisibleSitewideAlerts();
was replaced by
$alerts = $this->sitewideAlertManager->activeVisibleSitewideAlerts();
but a few lines below the variable name adjustment was not reflected, so the first argument to krsort isn't an array.
krsort($sitewideAlerts, SORT_NUMERIC);

dburiak made their first commit to this issue’s fork.

dburiak’s picture

Status: Needs work » Needs review

Applied @malcomio suggestion.

Tested on Drupal 10.4.8 - looks good to me. Please review.

smustgrave’s picture

Status: Needs review » Needs work

Still appears to be missing tests

mangesh.borukar made their first commit to this issue’s fork.

nala-he’s picture

I have manually tested the latest commit for MR!65. The patch works fine. We’ve also had this patch running on our production site for around 5 months without issues.

@smustgrave I'm new to contributing tests, but I am interested in helping to create them for this patch. Could you please clarify what types of tests would be helpful to advance this issue?

cyu’s picture

Noting that I've used this patch for some time now, but there have been occasional caching issues. Recently, I saw a scheduled alert linger for over 24 hours past the scheduled end time, only going away after a "Flush All Caches" run in Drupal. Usually this isn't much of an issue for my use cases, but I can see where that would make this unusable for others.

malcomio’s picture

The caching issues should be addressed by the latest commit in the merge request: https://git.drupalcode.org/project/sitewide_alert/-/merge_requests/65/di...

cyu’s picture

Thank you @malcomio. I was pretty sure that I had picked up those changes in my local patch, but upon review I see that I did not. I'll give that a go and report back if we continue to see any caching issues. (Turns out I had picked up the updates labeled "update caching" but the commit message on this most recent one of "Handling the display of the sitewide alert for the admin user" threw me off)

mohammedodeh’s picture

StatusFileSize
new23.53 KB

Reroll the patch to version 3.0.1

smustgrave’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

As noted we need to make this a configuration option

chrissnyder changed the visibility of the branch 3.1.x to hidden.

chrissnyder changed the visibility of the branch 2.x to hidden.

chrissnyder’s picture

Version: 3.0.x-dev » 3.1.x-dev
StatusFileSize
new13.43 KB

Thank you all for your efforts on this issue. I have created a new branch that consolidates all of this work, refactored some parts of the approach, added it as a configurable option, and included unit tests.

Sitewide alert config option for ssr

If this looks good I plan on releasing it as part of a 3.1.0-beta1 release.

chrissnyder’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Tested the MR for 3.1.x and worked like a charm.
Update hook ran without issue
Applying the setting the alerts are still appearing as before with 0 shift

  • chrissnyder committed 44fecf79 on 3.1.x
    Issue #3291075: Add option to render the alerts server-side, avoiding...

smustgrave’s picture

Status: Reviewed & tested by the community » Fixed

This was merged in 3.1.x thanks @chrissynder!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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