Improve cacheability

These issues can be solved the following way:

* The block should get a cache context that produces a unique hash representing the IDs of the visible alerts, or "none" if no alerts are visible. This will allow to cycle alerts without invalidating any existing page caches. The nice things about cache contexts is that if an existing alert is disabled and then enabled again at a later point, the cached versions will still be there. Even the full page cache can just be reused and doesn't need to be regenerated.
* The block should inherit the cache tags of the alerts that are shown in it. In that case if any of the alerts are edited only the cached versions of the block that actually contain this alert will be invalidated, and all cached versions of the block that don't contain it will be untouched.

With these two mechanisms in place the current approach of using AJAX to inject the content into a cached page is no longer needed.

Going to implement this.

Kicking this off with a failing test.

The test is failing as expected:

1) Drupal\Tests\site_alert\Functional\SiteAlertCacheTest::testPageCache with data set #0 (300)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'MISS'
+'HIT'

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:37
/var/www/html/modules/contrib/site_alert/tests/src/Functional/SiteAlertCacheTest.php:147

This indicates that when there is a site alert on the page and the page is cached, and a new scheduled site alert comes along, the cached version (without the scheduled alert) is still shown.

Now let's fix this by introducing a cache context that will create variations of the site alert block depending on which alerts are being shown.

Work in progress. My progress is being hindered by #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache which is making my planned approach impossible right now.

I have to work on some other priorities today, here is my to do list so I can pick this back up later:

diff --git a/src/Plugin/Block/SiteAlertBlock.php b/src/Plugin/Block/SiteAlertBlock.php
index 1f4c9ad..58e8ab8 100644
--- a/src/Plugin/Block/SiteAlertBlock.php
+++ b/src/Plugin/Block/SiteAlertBlock.php
@@ -176,6 +176,7 @@ class SiteAlertBlock extends BlockBase implements ContainerFactoryPluginInterfac
    * {@inheritdoc}
    */
   public function getCacheContexts() {
+    // @todo Also add a getMaxAge() even though this won't work right now.
     return Cache::mergeContexts(parent::getCacheContexts(), ['active_site_alerts']);
   }
 
diff --git a/src/SiteAlertStorage.php b/src/SiteAlertStorage.php
index e117499..1d397f5 100644
--- a/src/SiteAlertStorage.php
+++ b/src/SiteAlertStorage.php
@@ -29,6 +29,8 @@ class SiteAlertStorage extends SqlContentEntityStorage /*implements SiteAlertSto
    *   The remaining time in seconds, or -1 when no alerts are scheduled.
    */
   public function getRemainingTime() {
+    // @todo Should return -1 when no alerts are scheduled.
+    // @todo Add test coverage.
     $query = 'SELECT MIN(t) AS t FROM (SELECT scheduling__value t FROM ' . $this->getBaseTable() . ' WHERE scheduling__value > :current_time AND active = :active UNION SELECT scheduling__end_value FROM ' . $this->getBaseTable() . ' WHERE scheduling__end_value > :current_time AND active = :active) AS u;';
     return $this->database->query($query, [
       ':current_time' => $this->dateNow(),
@@ -43,6 +45,7 @@ class SiteAlertStorage extends SqlContentEntityStorage /*implements SiteAlertSto
    *   The date string representing the current time.
    */
   protected function dateNow() {
+    // @todo Move into a trait.
     $now = new DrupalDateTime();
     $now->setTimezone(new DateTimeZone(DateTimeItemInterface::STORAGE_TIMEZONE));
     return $now->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT);

Posting progress. I have looked into using the Cache Control Override module to see if it could help site builder get properly cached pages for anonymous users with an expiration date which matches the time the next alert would appear, but it doesn't work. This is probably due to the cache max age being ignored. I see the following mention in #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache even though the Cache Control Override module page doesn't mention this limitation:

CCO only detects max-age = 0 so it won't work if your block has a small positive max-age

In any case we should not introduce a dependency on CCO just to work around a core bug.

We can make this work by falling back to a JS driven approach in case the page cache is enabled.

Small cleanups.

Implemented the workaround. This still needs test love.

@pfrenssen I am concerned by this comment

"With these two mechanisms in place the current approach of using AJAX to inject the content into a cached page is no longer needed."

I think that would invalidate the value of this module. One of the things that makes this module so useful is that updates to alerts appear without needing a page reload. The AJAX is not simply to be a cache bypass, it is to provide real-time alerts. In my opinion the AJAX (or a socket) is very useful.

Example: An editor working on a long and complex page can get notified about downtime in real-time.

@swirt no worries, the idea was to make the AJAX refreshing optional rather than a hard requirement, it was never the intention to remove it. See #3111964: Make AJAX refreshing user configurable.

Thank you for clarifying this. Now I can rest easier ;)

Added test coverage for the method that returns the cache max age. Found a bug in the query thanks to the test: the base table name should be surrounded with curly braces.

Next up is to write a JS test for the page cache workaround.

Added the test coverage for the workaround to allow alerts to show on cached pages, until #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache is fixed.

This is now ready for review.

1. Fixed.
2. Fixed.
3. I learned this also while working on this issue. The documentation on CacheContextInterface::getCacheableMetadata() is not clear on this, but the idea is that some cache backends cannot realistically work with cache contexts; for example a static page cache intended for anonymous users, such as Varnish or the core page_cache. But these backends can set the cache lifetime. We can leverage this perfectly since our content is scheduled and we know exactly when to invalidate the caches.
4. Very good suggestion! I was so focused on the block that I didn't realize the AJAX response it not cached. I have implemented your suggestion.
5. Fixed.
6. Fixed.
7. I have given this some thought but I think we can let it pass, and maybe even a followup is not really required. The use case for this is quite unlikely: a site would have to create or change inactive alerts regularly throughout the day for this to become a problem. Then I realized that this would be the same for nodes: only published nodes would affect the render caches, and still the node list cache tag invalidates whenever an unpublished node changes. If it is not a problem for nodes, then probably also not for us :)

   Of course if someone is hit by this they are free to create an issue + patch.