-
Notifications
You must be signed in to change notification settings - Fork 8
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
Performance improvements. #321
Conversation
…nners method so it can be called multiple times without re-loading banners.
Thanks @rupertj. I'll take a look and test, seems straight forward. |
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. |
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. // 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. |
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...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Stopped block constructor loading entities. Changed getCurrentAlertBanners method so it can be called multiple times without re-loading banners.
Fixes #320