backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[PS][D8] Advanced Drupal Caching (a.k.a. cache tags and cache max-age) #4127

Open domaingood opened 4 years ago

domaingood commented 4 years ago

Description of the need A website’s cache is only useful if it has the necessary content on hand to satisfy visitors’ requests. Pantheon’s granular cache control enables developers and site owners to manage updates to specific assets, pages, categories, and even personalization across our Global CDN.

Your sites will maintain a high cache hit rate (a.k.a. fast page load speeds) while ensuring the latest content is always in the hands of visitors. Pantheon’s granular cache control prevents inadvertently invalidating an entire site’s cache. There is no need to throw out an entire cache every time you make small content changes on a site.

Proposed solution Need to add backdrop CMS https://www.drupal.org/project/d8cache


PR by @hosef: https://github.com/backdrop/backdrop/pull/3475

klonos commented 4 years ago

Thanks for filing this issue @domaingood 👍 ...I was not aware of that D7 contrib module!

The advanced D8 caching is always being brought up on discussions I am having with other people, so I would love it if we had that feature parity with D8; especially if it makes it one reason less for people being hesitant to adopt Backdrop.

I'm not an expert in this, so I am really interested to see what our core maintainers have to say.

klonos commented 4 years ago

I have added this issue in the "things to consider" list in #378 😉

jenlampton commented 4 years ago

I'd love to get cache tags into Backdrop. Posting this link to the Drupal 7 module that's a backport of Drupal 8 cache tags.

edit: oh. It was already in the issue summary... thanks @domaingood not sure how I missed that :)

hosef commented 3 years ago

I am going to take a stab at this. I suspect that this might break some APIs, but I will see how much I can implement without doing that.

hosef commented 3 years ago

I have made a port of the core functionality of the d8cache module and uploaded it at https://github.com/hosef/d8cache, although I have not fixed the hooks for blocks because they are completely different. The next step is to integrate the code into the existing cache system and make a PR.

Because we are changing the interface for the cache back ends, this will probably break external cache back ends, such as Redis or Memcache, that are provided by contrib, so we should make sure that those are updated in conjunction with this.

jenlampton commented 3 years ago

Because we are changing the interface for the cache back ends

Is it possible to make this change in a way that is backwards-compatible? I'd really like to see this in 1.x :)

hosef commented 3 years ago

I finally have all the tests passing, so this is ready for review.

I am pretty sure there is still stuff that i have to clean up in the comments and with coding standards, but I would like it if someone could do a more technical review on the implementation and see if there are any places where things could be improved.

klonos commented 3 years ago

@hosef does this aim to also add support for libraries/assets and configuration, same as D8? See: https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags#syntax

Examples:

...

  • ...
  • config:system.performance — cache tag for the system.performance configuration
  • library_info — cache tag for asset libraries
klonos commented 3 years ago

...I don't see any ::getCacheTags() or ::getListCacheTags() methods either.

klonos commented 3 years ago

...also, interesting change record, with follow-up issues: Added an ENTITY_TYPE_list:BUNDLE cache tag

Note: There is currently no API to get per-bundle and more specific cache tags from an entity or other object. That is because it is not the entity that decided which list cache tags are relevant for a certain list/query, that depends on the query itself. Future Drupal core versions will likely improve out of the box support for per-bundle cache tags and for example integrate them into the entity query builder and views.

herbdool commented 3 years ago

@hosef I've reviewed the code but haven't tested. I'll try test it soon too. My idea of testing is to create a view with caching enabled and then update some nodes that are included in the view. And see if the view cache gets updated. Make sense?

quicksketch commented 3 years ago

@hosef requested to take himself off this issue for now. It's not going to be something that's together for 1.20 for sure.

domaingood commented 3 years ago

All checks have passed

hosef commented 2 years ago

I think I am going to take another stab at this. I want to try and see if I can implement it in a way that is easier to explain and understand.

domaingood commented 1 year ago

Add Redis For Backdrop CMS https://redis.com/lp/drupal-redis/

herbdool commented 7 months ago

@hosef I think the PR is close. I suggested a fix for when someone is doing an upgrade, which is why a couple tests are failing. I was suggesting creating the db table much earlier, but even then we don't know where someone might be upgrading from. So thinking now, there should be a check before trying to update the table.

hosef commented 1 month ago

@herbdool I updated the PR with the changes from 1.x again and I think I have addressed all of the things you brought up in the last review.

There is now a new issue though that I have not figured out yet. It seems that a few tests are passing invalid cache ids into getMultiple() which is then throwing an Exception. This seems to happen randomly on Github, and doesn't happen at all on my local, so it has been a challenge to track down so far.

hosef commented 1 month ago

I was able to get rid of the issue by totally removing the call to array_flip. I was not able to find any other way of getting that error to go away nor any way to print out values that would cause the error.

argiepiano commented 1 month ago

nor any way to print out values that would cause the error.

@hosef, not sure if you are aware, but the automated tests now save the verbose output as "artifacts", available by clicking on the workflow name at https://github.com/backdrop/backdrop/actions. A possible way to debug values is to invoke the method verbose() with those values, then checking the artifact.

Screen Shot 2024-09-05 at 1 33 24 PM
hosef commented 1 month ago

That is good to know. The problem though was that as far as I could tell, my debug code just never ran at all. Even putting a try/catch around it didn't stop it from throwing an exception .