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

Simplify refreshing a template’s cache #3880

Open
MatTheCat opened this issue Sep 17, 2023 · 11 comments
Open

Simplify refreshing a template’s cache #3880

MatTheCat opened this issue Sep 17, 2023 · 11 comments

Comments

@MatTheCat
Copy link
Contributor

MatTheCat commented Sep 17, 2023

https://twig.symfony.com/doc/3.x/recipes.html#using-a-database-to-store-templates shows how to load templates from a database, but following this recipe will make templates’ source update inoperant by default when debug mode is disabled.

You can enable auto_reload to work around the issue, but this will cause every loader to check for cache freshness every time a template is rendered.

I think it would be more efficient to refresh a template’s cache after its source is updated, but the corresponding code is somewhat complex and needs the @internal Environment::getTemplateClass method:

$twigCache = $twig->getCache(false);

$twigCache->write(
    $twigCache->generateKey($templateName, $twig->getTemplateClass($templateName)),
    $twig->compileSource($twig->getLoader()->getSourceContext($templateName))
);

Would it make sense for Environment to expose something like a refreshCache(string $templateName) method?

@kevinpapst
Copy link

I struggled with that problem for a while and the solution is simpler. When someone changes a template, you can do this:

    $twig->enableAutoReload();
    $twig->load('@bundle/' . basename($template->getFilename()));
    $twig->disableAutoReload();

You just need to pass in the correct filename used by your twig loader.

But I would also like to have an explicit refreshCache(string $templateName) method, which takes care of possible race conditions and such.

Also this should be documented somewhere. I am ashamed to share how long it took to figure out these 3 simple lines.

@MatTheCat
Copy link
Contributor Author

In your case the cache being refreshed is a side effect of your code so it feels even more hackish to me 😅

I guess I’ll try opening a PR this week.

@kevinpapst
Copy link

I disagree 😁 Your code shows that you need to have extreme detail knowledge of the internals to solve it otherwise.
But hack'ish or not, as long as there is no dedicated public API, we need to help ourselves somehow, right 🤷

Anyway: I think we agree that there should be a better way. And according to Stackoverflow many people think the same.
Would you please send me a ping when you prepare a PR? I'd love to test!

@MatTheCat
Copy link
Contributor Author

Damn there’s something I haven’t thought of: if a template has already be loaded, then calling “refreshCache” would mean you cannot have the updated version in memory (because AFAIU the cache key can be independent of the source, and you cannot redefine a class at runtime).

I think that means this cannot be implemented in Twig itself. Would be happy to be proved wrong though.

@kevinpapst
Copy link

kevinpapst commented Oct 3, 2023

There might be circumstances where it doesn't work 🤷 but in all prod environments I tested it works fine with opcache turned on to max cache settings (don't check file again, no reload: keep everything in memory until restart).

The filesystem cache does it this way:
https://github.com/twigphp/Twig/blob/3.x/src/Cache/FilesystemCache.php#L64

If autoReload() works, then why should a single method call not work?
It might suffer from the same limitations (if there are some), but it is still worth to be added IMO.

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Oct 4, 2023

If autoReload() works, then why should a single method call not work?

I think it won’t work if the template already has been loaded, because in that case loadTemplate would return before any interaction with the cache.

@kevinpapst
Copy link

Hm, I tested again. Maybe it really doesn't work. I need to do some more testing. I was sure that it reloaded the cache, but I haven't checked the entire flow in quite a while... but how is it that the auto reload works in dev mode?
Maybe that is a way to trigger the reload?

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Oct 4, 2023

Auto reload only works when a template is loaded for the first time; after that it will be kept in memory.

That’s what I think is an issue with my proposal: it refreshes the cache, but not the template in memory in case it has already be loaded. It doesn’t matter in my case but this could be a problem in another.

@stof
Copy link
Member

stof commented Oct 4, 2023

There is no solution for replacing an already loaded template for the current PHP process. That would require a full rewrite of Twig (and probably sacrificing some performance for all projects that don't need to support this refreshing feature as we would loose support for preloading compiled template classes if we stop generating named classes).

If you really want to change the template dynamically, maybe your use case is for Environment::createTemplate instead.

@MatTheCat
Copy link
Contributor Author

Yeah we somewhat drifted but the original goal was to be able to refresh a template's cache 😅

Do you think the code I posted in the issue could be integrated to Environment? Or would it exist a better way?

@kevinpapst
Copy link

It was never about changing the template in one request (at least not for me). I think the default use-case is that a user updates a custom template and then it should be recompiled to be used later (not the same request).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants