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

[WIP] feat: use backend handling for cookie check #306

Draft
wants to merge 3 commits into
base: 1.x
Choose a base branch
from

Conversation

millnut
Copy link
Member

@millnut millnut commented Feb 5, 2024

No description provided.

@andybroomfield
Copy link
Contributor

andybroomfield commented Feb 5, 2024

What I currently see, the banner reappears when hidden.

Alert.banner.cookie.backend.p1.mp4

This is with the new template addition in localgov_base and with internal page cache turned on.
Since anon users always get the same rendered page, the banner is always shown. Clearing the cache and refreshing does hide it, but it hides it for all anon users.

@andybroomfield
Copy link
Contributor

This will be Drupals Internal page cache which does not vary the page by cookie. It's only possible to bypass this by throwing the page cache kill switch with \Drupal::service('page_cache_kill_switch')->trigger(); and any page with an alert banner would not be able to benefit from caching the rendered html to anon users. Given that the alert banner is quite often used by every page, I think this would be an hard performance hit, especially on smaller councils on non enterprise hosting, and even BHCC this would be a performance hit.

@millnut
Copy link
Member Author

millnut commented Feb 6, 2024

Agreed @andybroomfield I spent some time looking into this more and trying different ways to invalidate and always hit the Drupal Internal Page Cache module for anonymous users. Some places recommend disabling this but I don't recommend this due to the negative performance impact you mentioned.

Thanks for testing this @andybroomfield I'll close/delete this PR later today and look into the client-side changes more.

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 this pull request may close these issues.

None yet

2 participants