Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Regression] Alert banners are no longer showing correctly when pages are cached #327

Closed
andybroomfield opened this issue May 31, 2024 · 0 comments · Fixed by #328
Closed

Comments

@andybroomfield
Copy link
Contributor

andybroomfield commented May 31, 2024

Looks like a regression introduced in #325

The removal of the getCacheContexts method on the alert banner block is sometimes preventing banners from displaying. Looks like this is a partular issue where Varish is used, but could also be the internal page cache.

Becuase of #154 we don't add the url.path cache context unless the banner appears on a page, otherwise the cache table will fill up with every page. But this means that the render array that is cached will be the one without this context unless its on the right page.

It looks like getCacheContexts does get called and the value stored in the cache table, wheras doing this in build does not. I think at least for now we should readd getCacheContexts method. It will still call $this->getCurrentAlertBanners() but I don't see that as a major issue unless there are lots and lots of live banners.
(The banner entity loading is cached, so its just the query I think is not. Note that trying to store the banner as a protected property won't work as the block gets reconstructed between then getCacheContexts and build method).

Long term we should move to rendering the alerts through big pipe so the banner block is just a placeholder.

andybroomfield added a commit that referenced this issue May 31, 2024
Fix #327

Part revert #325
Restores the getCacheContexts method as that is what is called each time
to determine the cache contexts each time the block is rendered.
andybroomfield added a commit that referenced this issue May 31, 2024
Fix #327

Part revert #325
Restores the getCacheContexts method as that is what is called each time
to determine the cache contexts each time the block is rendered.
andybroomfield added a commit that referenced this issue May 31, 2024
Fix #327

Part revert #325
Restores the getCacheContexts method as that is what is called each time
to determine the cache contexts each time the block is rendered.
andybroomfield added a commit that referenced this issue Aug 29, 2024
See #327.

Extends visibility test by providing actual pages for the banner to display on.
This also provides an extra banner to test the correct banner is visible per page.
andybroomfield added a commit that referenced this issue Aug 29, 2024
See #327.

Extends visibility test by providing actual pages for the banner to display on.
This also provides an extra banner to test the correct banner is visible per page.
andybroomfield added a commit that referenced this issue Sep 3, 2024
See #327.

Extends visibility test by providing actual pages for the banner to display on.
This also provides an extra banner to test the correct banner is visible per page.
andybroomfield added a commit that referenced this issue Sep 12, 2024
See #327.

Extends visibility test by providing actual pages for the banner to display on.
This also provides an extra banner to test the correct banner is visible per page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant