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:
- Special internal routes and aliases such as
/node/2,<front>aren't supported. Routing conditions should ideally be handled in the backend - 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
- 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
| Comment | File | Size | Author |
|---|---|---|---|
| #45 | Sitewide_Alert_Global_Settings___Sitewide_Alert_Test.png | 13.43 KB | chrissnyder |
| #38 | 3291075-38.patch | 23.53 KB | mohammedodeh |
| #24 | 3291075-24.patch | 23.77 KB | qusai taha |
| #21 | 3291075-21.patch | 26.25 KB | qusai taha |
| #18 | 3291075-18.patch | 61.89 KB | qusai taha |
Issue fork sitewide_alert-3291075
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
Comment #2
chrissnyderComment #3
greg boggsI 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.
Comment #4
malcomio commentedRegarding 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.
Comment #5
chrissnyderI 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.
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.
Comment #6
greg boggsThe 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.
Comment #8
malcomio commentedWork 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
drupalSettingsThis probably needs further thought from a security and performance point of view.
Also, as per #5, ideally we would make this configurable.
Comment #10
mlncn commentedWould absolutely love this with the further enhancement that if "Automatically Update (Refresh) Alerts" (polling) is disabled in
/admin/config/sitewide_alertsand "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.Comment #11
chrissnyderWould 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?
Comment #12
mlncn commentedComment #13
aron novakWe tested it on a client project, worked well.
Comment #15
rpayanmI added some suggestions and fixed the test error. Please review.
Comment #16
ibullock+1 for RTBC, been using in the wild for a few months now without issue.
Comment #17
doxigo commented+1 for this. works as expected
Comment #18
qusai taha commentedCreate a patch from the MR
Comment #19
doxigo commented@qusai taha, you can just click on the plain diff and that's a patch already, no need for a separate patch
Comment #20
qusai taha commentedRe-roll the patch to make it working with latest version
Comment #21
qusai taha commentedRe-roll the patch to make it working with latest version
Comment #22
smustgrave commentedComment #23
qusai taha commentedRe-roll the patch to make it working with latest version
Comment #24
qusai taha commentedComment #25
smustgrave commentedFixes should be in MRs not patches just fyi
Comment #28
edwardsay commentedMR for version 3 https://git.drupalcode.org/project/sitewide_alert/-/merge_requests/65
Comment #29
cyu commentedThanks 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);Comment #31
dburiak commentedApplied @malcomio suggestion.
Tested on Drupal 10.4.8 - looks good to me. Please review.
Comment #32
smustgrave commentedStill appears to be missing tests
Comment #34
nala-he commentedI 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?
Comment #35
cyu commentedNoting 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.
Comment #36
malcomio commentedThe caching issues should be addressed by the latest commit in the merge request: https://git.drupalcode.org/project/sitewide_alert/-/merge_requests/65/di...
Comment #37
cyu commentedThank 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)
Comment #38
mohammedodeh commentedReroll the patch to version 3.0.1
Comment #40
smustgrave commentedComment #41
smustgrave commentedAs noted we need to make this a configuration option
Comment #45
chrissnyderThank 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.
If this looks good I plan on releasing it as part of a 3.1.0-beta1 release.
Comment #46
chrissnyderComment #47
smustgrave commentedTested 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
Comment #50
smustgrave commentedThis was merged in 3.1.x thanks @chrissynder!