Move the cacheContexts and cacheTags to the alert banner entity

[andybroomfield]

Fix #150

• Remove the session and user role contexts
• Move cache contexts to alert banner entity
• Move cache tags to alert banner entity

[andybroomfield]

Initial fix.
Let me know if this works for you @Adnan-cds.

I was a little confused at the cacheContexts in the alertbanner entity. Should I be adding them via $this->addCacheContexts($contexts); or merging them with the parent tags as in return Cache::mergeContexts(parent::getCacheContexts(), $contexts);

[Adnan-cds]

Code looks good Andy. I will try it and get back to you. Thanks a lot for the prompt action :)

[stephen-cox]

@andybroomfield I've made a few comments. I'm not an expert on Drupal cache contexts and tags, so am happy to be overidden on some of the suggested changes. But having thought it through I'm not sure when I originally added code to the getCacheContexts method it was the best place for it. It may be better being called by the constructor. Happy to discuss further.

[stephen-cox]

I don't think this line is necessary. Cache contexts are just strings so you can initialise it with $contexts = ['url.path.is_front'];

[stephen-cox]

As we're setting cache contexts outside the visibility field check I think this line should be moved out of the check block. Although this might not be needed at all.

[Adnan-cds]

Although this might not be needed at all.

Seem to be correct.

[stephen-cox]

This can be simplified to $cache_tags = ['localgov.alert.banner.token:' . $token];

[Adnan-cds]

Hi Andy,
I have finished my testing. It was doing great until I added a Visibility condition that says "Do not display this alert on the homepage". After that, every page started generating a cache entry in the cache render table. Example cache key: entity_view:block:localgov_alert_banner:[languages:language_interface]=en:[theme]=localgov_theme:[url.path]=/1524:[user.permissions]=71...

The url.path cache context may be coming from the condition_field module. In which case we probably can't do much.

In contrast, when I changed the visibility rule to only display that alert on the homepage, there were only two cache keys in the cache_render table irrespective of how many pages I visited. That's good news :)

Given all this, I think I can live with PR. We are not even using any Visibility rule in our site given that this is so new :)

[Adnan-cds]

By the way, I noticed something very odd about the Visibility rule. It is showing up as part of the Alert banner! I have attached a screenshot:

Possibly caused by this piece of config

[andybroomfield]

@Adnan-cds I noticed that and thought I upgraded wrong so removed it in managed display.
I'd start a new PR that moves that config to disabled

[Adnan-cds]

I'd start a new PR that moves that config to disabled

Thanks Andy :)

[andybroomfield]

Have now updated this branch.
Following @Adnan-cds suggestion, we're using the method that Drupal core and other modules use, which is to merge the context provided by the alert banner entity with that provided by the parent::getCacheContexts. This should still allow other modules to add custom cache contexts and tags as they will still be retrived from entityBase.php

[Adnan-cds]

I am happy with what I am seeing.

[andybroomfield]

@stephen-cox are you happy with this change also, as its your request that is blocking?