Make cache extension optional and let users choose their own implemen…

[nlemoine]

Issue

Whatever comes out of #2407, I think users should be able to choose their cache implementation.

Solution

Add a filter to control Timber Cache implementation.

Impact

None because it defaults to true.

Usage Changes

Users can use that filter to manually register their own cache extension.

Considerations

use twig/cache-extra 😉

[jarednova]

Based on my reading from #2407 this feels like a logical next step to both deal with the abandonment of that package while providing more user control. At minimum, this gives users who don't want an obsolete library added to their Twig instance the ability to control.

From what I gather from @gchtr's comment — the work to fully remove and upgrade aren't insignificant. I move that we call that a non-blocking "2.x Future" task. I'd want @gchtr's review and input rather than review/approval from me.

[gchtr]

Well, I can’t say too much about everything that’s involved, because I didn’t work often enough with Timber’s caching mechanism. But I feel like this could hold us back even more, despite the very valid points that @nlemoine raises in #2436 (comment).

For this pull request, I just have a suggestion for the naming of the filter, otherwise this sounds like a good compromise for the moment.

[nlemoine]

I can’t say too much about everything that’s involved, because I didn’t work often enough with Timber’s caching mechanism

Neither do I, but from I've read in the Twig cache docs and the Symfony cache docs, the cache strategy (e.g. KeyGeneratorInterface based strategy) seems kind of outdated now. This goes in the right direction to me, leaving developer the responsability (and freedom) to set their own auto expiring cache keys.

Timber is about bringing Twig (and many other good practices) in WordPress, if some users wants extra features that are outside the Twig base scope, Twig already has reliable/heavily tested extensions you can freely add to your Timber environment.

So why not completely remove this extension? After all, the cache extension is the only one added by default in Timber. There are also some handy ones I use in all my projects. However, I think they belong to the right place: in my personal project composer.json. Maybe the cache extension should be too.

Here's how I see this breaking change for 2.0:

• Fully remove the cache extension from Timber
• Add twig/cache-extra to the suggest field of composer.json
• Add a basic example showing how to enable the cache-extra extension (basically what's in the end of that page)
• Release a transient adapter in 2.x future

Otherwise, once 2.0 is out we'll be stuck with the twig/cache-extension composer dependency/code and will make everything harder.

[gchtr]

Here's how I see this breaking change for 2.0:

* Fully remove the cache extension from Timber

* Add twig/cache-extra to the `suggest` field of `composer.json`

* Add a basic example showing how to enable the cache-extra extension (basically what's in [the end of that page](https://twig.symfony.com/doc/3.x/tags/cache.html))

* Release a transient adapter in 2.x future

Otherwise, once 2.0 is out we'll be stuck with the twig/cache-extension composer dependency/code and will make everything harder.

I think this is a fine plan 👌. Allowing users to add their own extensions is also what we do with Routing. We could still add the relevant documentation for how to add the cache extension.

I just want to hear what @jarednova is thinking about this.

[jarednova]

I'm so sorry @nlemoine @gchtr for letting this hang. Didn't realize there was an open Q for me. I agree with the findings, but differ on the timing.

Otherwise, once 2.0 is out we'll be stuck with the twig/cache-extension composer dependency/code and will make everything harder.

I realize that the suggested changes to the cache extension are breaking change. But rather than hold 2.0 up, couldn't this be in 3.0 soon after? I worried we've gotten into a bad habit of trying to get all the breaking changes in 2.0, when in fact we can accelerate the release cycles so each version change doesn't have to "wait" on everything being ready.

[nlemoine]

Thanks @jarednova for coming back on this topic. I'm fine with breaking changes. However, can you approve this PR? It doesn't introduce a BC and still gives the freedom to opt in for a different cache extension.

[jarednova]

Yep sounds good @nlemoine — so just to wrap up where we're landing: we'll pull in this PR; but make the subsequent suggested PR (of a more complete removal of the cache extension) for a 2.x Future / Breaking Change issue & PR.

This has my ✅ ; but actually @gchtr's is needed to merge as code owner of the 2.x branch / accepting the requested changes.