Photoswipe requires Dynamic Page Cache without adding it as a dependency

Thanks for the report @imclean!

I agree this is wrong and the explicit call should be removed! For the next step we need to find out, which caches really need to be cleared or if some cache related stuff is missing somewhere else in the module? Perhaps compare implementation to similar modules like colorbox?

Could you help?

Fixed and tested, works like a charm. Please review!

Thanks @Grevil! That's at least a good quick fix. @imclean can you confirm it works for you and fixes the issue?

@Grevil: I think it would be even better (perhaps as X-linked follow-up) if we wouldn't interact with dynamic page cache directly in any way.
My hope is that there's a more generic cache clear command from core, which dynamic page cache and possibly other caches attach to and get notified, if cleared.

I think our best chance is to look into the dynamic page cache module itself or other modules clearing caches in similar cases that will have the same issues?

Update: Reading DynamicPageCacheSubscriber it looks like we "just" need to invalidate relevant cache tags instead and the dynamic page cache won't return his "old" entries anymore.

Perhaps

\Drupal\Core\Cache\Cache::invalidateTags(['rendered']);

while that might be need to be quite broad in the context of Photoswipe, but the current calls are also very broad!

I think that should allow us to get rid of both lines:

    $this->renderCache->invalidateAll();
    $this->dynamicPageCache->invalidateAll();

Asking "Is there a broad core cache tag to invalidate for all pages?" the AI says:

Yes, there is a broad core cache tag that you can use to invalidate the cache for all pages. You can use the `rendered` cache tag². Here's how you can do it:

```php
\Drupal\Core\Cache\Cache::invalidateTags(['rendered']);
```

This line of code will invalidate the cache of every rendered page². However, please be aware that this is a sledgehammer solution and the performance on your site will take a hit, particularly if you have a lot of nodes². It's recommended to use this method judiciously and only when necessary².

Quelle: Unterhaltung mit Bing, 10.1.2024
(1) How to programmatically invalidate the cache of every node. https://drupal.stackexchange.com/questions/314502/how-to-programmaticall....
(2) Cache tags | Cache API | Drupal Wiki guide on Drupal.org. https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags → .
(3) Cache::invalidateTags | Cache.php | Drupal 10.1 | Drupal API. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C....
(4) 8 - Using cache tags for a particular node list - Drupal Answers. https://drupal.stackexchange.com/questions/200321/using-cache-tags-for-a....
(5) Don't invalidate cache tags of referenced entities, use entity ... - Drupal. https://www.drupal.org/project/drupal/issues/2304987 → .

Do we have a test in place that already fails without clearing the cache correctly? That would be fabulous!!

Not a direct test, but based on the comment here: https://www.drupal.org/project/photoswipe/issues/3021978#comment-15196157 ✨ [5.x] Add photoswipe options UI (and optionally setting presets) Fixed

The tests should fail, if we do not do a proper cache clear, so let's try that instead!

Need to create a second branch to test @Anybody's approach, since local testing through PHPUnit is still broken for Photoswipe for unknown reasons. Problem is, that the tests fail, because the Mink Driver Library has changed and we can not pass int values inside "fillField".

So gotta fix the tests first.

Alright, tests go green!

I'll remove the use of the services entirely, afterwards this can go back to RTBC! I hope that tag is really enough, but we have pretty good test coverage for this module, so I'd say we make the change.

Grevil → closed merge request !112

Wow, this change speeds up saving the configuration around 1000% AND seems to work, when manually testing it! Thanks a lot @Anybody! 😁

Tests all green again, please review!

Yeah super nice! We should add a lot of caching documentation to our snippets :D

RTBC from my side. Would be happy to also hear @imclean's feedback!

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