Skip to content
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

Preview tokens 6: Version-based page caching #6827

Open
wants to merge 1 commit into
base: v5/develop
Choose a base branch
from

Conversation

lukasbestle
Copy link
Member

Description

Summary of changes

This PR makes the pages cache behave well with the new $versionId param for $page->render(), even when e.g. generating a static site manually.

Reasoning

Updating a changes version does not flush the pages cache (would be pretty wasteful), so rendered changes versions should also never be cached.

Extra logic is needed because one can now manually pass the changes version to render, even if no request param is set (which would also prevent caching with existing logic).

Cache ID is already changed because v5 is a good opportunity to do so. This prepares the code for future additions to our version types (e.g. revision support).

Additional context

The breaking change also affects our Staticache plugin. We need to release a version for v5 that supports the version addition to the cache ID.

Changelog

Enhancements (since alphas)

  • The pages cache only caches the latest version, not the changes version. This is because updating the changes version does not flush the pages cache. So changes previews are always rendered live.

Breaking changes

  • The cache IDs in the pages cache now include a part for the version ID. Currently this is always .latest, e.g. home.latest.html.

Docs

None

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@lukasbestle lukasbestle added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Dec 2, 2024
@lukasbestle lukasbestle added this to the 5.0.0-beta.1 milestone Dec 2, 2024
@lukasbestle lukasbestle self-assigned this Dec 2, 2024
Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all right to me - maybe @bastianallgeier also has a look

Updating a changes version does not flush the pages cache (would be pretty wasteful), so rendered changes versions should also never be cached.

Extra logic is needed because one can now manually pass the changes version to render, even if no request param is set (which would also prevent caching with existing logic).
@lukasbestle
Copy link
Member Author

Just rebased against v5/develop without any changes to the commit itself. Needed to do that so the diffs for following PRs line up.

Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me too. Just one question: could we omit the additional version part in the cache id for the latest version? Then the old cache entries could still stay valid. It's just a thought though. I don't mind the breaking ids too much.

@lukasbestle
Copy link
Member Author

Hm, good question.

In Staticache we currently parse the cache ID by splitting it with explode('.'). If caches for the latest version omit the extra suffix, Staticache (and others) would need to handle both cases. Of course not in v5, but in the future when there are other version IDs.

I wonder if this could cause ambiguity, e.g. if a UID contains a dot for whatever reason. Then Staticache could not detect what is the UID and what the version ID. Not sure if we need to support that (I think the Panel doesn't allow pages with dot in the UID), but in the filesystem it could exist and cause a complexity nightmare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants