Performance improvements.

[rupertj]

Stopped block constructor loading entities. Changed getCurrentAlertBanners method so it can be called multiple times without re-loading banners.

Fixes #320

[andybroomfield]

Thanks @rupertj. I'll take a look and test, seems straight forward.
Do we actully need to store the results of getCurrentAlertBanners() at all. If its caching isn't that what entitiy type manager is meant to do.
I've been meaning to move the logic into a service, and maybe use Big pipe to render them, which might simplify things.

[rupertj]

Thanks for looking @andybroomfield :) My understanding is that the entity storage doesn't cache results of entity queries. Even if it does, we might as well save the cache lookup and keep the results around, as we're guaranteed to need them again to build the cacheability metadata.

[andybroomfield]

Thanks @rupertj. The changes make sense, removing the call to getCurrentBanners in the constructor, but I wonder if we can actully elimnate the double call for cache tags.
In BHCC, a lot of our custom blocks use the pattern

// Add cache tags and context to back link block.
$build['#cache']['tags'][] = 'origin_node_id:' . $origin_node_id;
$build['#cache']['contexts'][] = 'url';

and dont include a getCacheTags() and getCacheContexts methods. Could a simmilar thing work here?

.. Looking at it more deeply, I see theres a bit of complexity in the getCacheContexts method that I need to look at to see if thats the best solution. I'm also wondering why the cacheContexts don't bubble up from the alert banner entities themselves.

[rupertj]

You make a good point. I'd probably do the cache metadata like this if I was starting from scratch: https://github.com/localgovdrupal/localgov_publications/blob/bf29b24b406324f4b9294f1a67756a3a5272201d/src/Plugin/Block/PublicationPageHeaderBlock.php#L88

(Why there's so many different ways to do it, I don't know...)

[andybroomfield]

For context, this is from @stephen-cox in #142 https://github.com/localgovdrupal/localgov_alert_banner/pull/142/files#diff-7d9b5a29e01a8d4067647507fb8f4f7af33c59befe9d876f725ae450096fef68

[andybroomfield]

I've being trying to see if there is a way to refactor without success. So if this is working for others I'd be happy to merge now, and I'm going to look at refacoring the logic when we move this to a service.