[austinderrick]

Summary

Cms\Twig\Loader::getCacheKey() returned the object's file path. Twig names the compiled template class after the cache key, and once that class is loaded it skips the freshness check (Twig\Environment::loadTemplate() only checks isTemplateFresh() when the class does not exist yet). A process that has rendered a page, layout, partial or content file therefore keeps rendering the old version after the object is edited, until the process ends.

Under PHP-FPM the process ends with the request, so nobody sees this. Under a persistent worker such as Laravel Octane, an edit made in the CMS editor never reaches a worker that already rendered the object.

The loader now appends a hash of the object's Twig content to the key, so an edit produces a new class name and Twig compiles the new version. The hash is used instead of the modification time because the modification time only changes once per second, so a second save in the same second would still render stale. Building the key in the loader also covers component partials and any cmsObject.getTwigCacheKey listener that returns its own key.

One side effect: when the Twig cache is on, each edit writes a new compiled file instead of overwriting the old one. Old files remain until the cache is cleared, as other cache files do.

Testing

testEditedObjectRendersItsNewContentInTheSameProcess in modules/cms/tests/classes/CmsObjectTest.php renders a theme file through a Twig environment with a filesystem cache and auto_reload, as Cms\ServiceProvider configures it. It then saves an edit to disk, reloads the object and renders it again in the same process, within the same second. It fails without the change. The CMS (220 tests) and System (348 tests) module suites pass.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7da5186d-b2a2-46ed-8418-3a3a73bbfce4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[LukeTowers]

That test doesn't look right. I don't see what's persisting the change and it's explicitly disabling the cache.

[austinderrick]

That test doesn't look right. I don't see what's persisting the change and it's explicitly disabling the cache.

@LukeTowers : Test updated! :)

[bennothommo]

@austinderrick sorry, but I'm not a fan of this change.

This change would mean a file read every time the system queries a template's cache key. I'm presuming when multiple templates are being read on a page, that issue would be multiplied. That would undoubtedly slow performance.

[austinderrick]

@austinderrick sorry, but I'm not a fan of this change.

This change would mean a file read every time the system queries a template's cache key. I'm presuming when multiple templates are being read on a page, that issue would be multiplied. That would undoubtedly slow performance.

Hey @bennothommo : I gave this another look and I'm pretty sure there isn't an extra file read. getTwigContent returns the object's markup (or content), which Halcyon has already loaded into memory by the time Twig asks for the key, and getSourceContext() already calls the same method. Am I missing something?

For context, this only matters for runtimes that keep PHP processes alive between requests, such as Octane, FrankenPHP worker mode or RoadRunner. Once a worker has loaded a template's compiled class, Twig never checks freshness again, so edits made in the CMS editor don't show up on that worker until it restarts. PHP-FPM never hits this because each request gets a new process.

The cost is one xxh128 hash over the in-memory markup per template load. I benchmarked the real Cms\Twig\Loader on PHP 8.4 (Linux, OPcache on) running each version in its own process and alternating the order:

• getCacheKey() takes about 1 µs longer per call, against roughly 53 µs for the existing call. Hashing alone takes 0.8 µs for a 10 KB template and 15 µs for a 200 KB one.
• A full page render (page, layout and 7 partials, so 9 key lookups) takes about 37 ms either way. Over 12 runs of 300 renders, the median was 37.0 ms before and 36.7 ms after, so the expected 9 µs is lost in the run-to-run noise.
• A partial rendered in a loop pays the cost on each render, so 200 renders of a small partial add about 0.2 ms.

I'm happy to share the benchmark scripts. If you'd still rather not change the default for everyone, I could put the content hash behind a config option that persistent-worker setups trigger.

[bennothommo]

@austinderrick I had assumed getTwigContent() reads the file, but I was wrong.

Your change makes sense, however, I would rather it be made in the getTwigCacheKey() method in modules/cms/classes/CmsObject.php, as opposed to just being tacked on to the end of the getCacheKey() method. The getTwigCacheKey() method has an event which allows developers to overwrite the cache key - that functionality still needs to be maintained, including overwriting your new hash if need be.