Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don't limit element cache by config cacheDuration #16796

Open
wants to merge 2 commits into
base: 6.x
Choose a base branch
from

Conversation

timkelty
Copy link
Contributor

Description

Limiting the element cache duration by \craft\config\GeneralConfig::$cacheDuration (defaults to a 1 day) can result in element caches being set with unexpectedly short expiry.

It seems better and more flexible to just let it stopCollectingCacheInfo return a null duration, and leave it to the caller to limit the value if desired.

@brandonkelly brandonkelly changed the base branch from 4.x to 4.15 February 27, 2025 23:16
@mmikkel
Copy link
Contributor

mmikkel commented Feb 28, 2025

The way I'm reading this change, template caches created with {% cache %} and containing element queries will no longer default to expire as per the cacheDuration config setting?

If that's the case, this feels like a pretty major behavioural change IMO.

@timkelty timkelty marked this pull request as draft February 28, 2025 14:26
@timkelty timkelty force-pushed the feature/dont-limit-element-cache-by-config branch from 5a84ee2 to 20f8452 Compare February 28, 2025 16:02
@timkelty
Copy link
Contributor Author

The way I'm reading this change, template caches created with {% cache %} and containing element queries will no longer default to expire as per the cacheDuration config setting?

If that's the case, this feels like a pretty major behavioural change IMO.

@mmikkel In that case, the template cache would be set with a null duration,, which would still cause it to default to \craft\config\GeneralConfig::$cacheDuration.

@mmikkel
Copy link
Contributor

mmikkel commented Feb 28, 2025

Aha! Good to know, thanks @timkelty – and pardon the noise 🙂

@timkelty timkelty changed the base branch from 4.15 to 5.x February 28, 2025 16:34
@timkelty
Copy link
Contributor Author

Decided to target this to 6, as it technically is a breaking change in this scenario:

An element has an expiry longer the configured $generalConfig->cacheDuration

  • The current (odd) behavior is to limit that to $generalConfig->cacheDuration.
  • The breaking change will just allow the duration to be set to what the element dictates.
  • In either case, default template cache is not affected because the cache component has a configured default of $generalConfig->cacheDuration.

@timkelty timkelty force-pushed the feature/dont-limit-element-cache-by-config branch from 20f8452 to 84805c8 Compare February 28, 2025 16:45
@timkelty timkelty mentioned this pull request Feb 28, 2025
@timkelty
Copy link
Contributor Author

Aha! Good to know, thanks @timkelty – and pardon the noise 🙂

No sweat – it made us dig deeper and realize it technically is a behavior change still, so will wait for 6.

@timkelty timkelty changed the base branch from 5.x to 6.x February 28, 2025 19:35
@timkelty timkelty marked this pull request as ready for review February 28, 2025 19:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants