Code

Opened 8 months ago

Closed 6 months ago

Last modified 6 months ago

#20945 closed New feature (fixed)

Allow cache tag to use specified cache config.

Reported by: FunkyBob Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently the cache tag will use the default cache - always.

However, the staticfiles app, for instance, can be configured as to which it will use.

I propose the following:

  1. Add a new setting (called DEFAULT_TEMPLATE_FRAGMENT_CACHE in the PR) to specify which to use by default.
  1. Extend the cache tag to allow the last argument to be using="cache name", falling back to the default if it doesn't exist.

Current code is:
https://github.com/django/django/pull/1490

I'm just working on the tests and final documentation updates.

Attachments (0)

Change History (15)

comment:1 Changed 8 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

You should probably raise the idea of adding a new setting on django-developers as we are averse to more settings and I'm not sure using a different cache than the "default" cache would be common enough to warrant it. If you have a use case for it, it would be helpful to describe it so we have some justification.

It may also be better if using="cache name" throws an error if the given cache doesn't exist (i.e. "fail loudly") -- what do you think?

comment:2 Changed 8 months ago by carljm

I don't think it makes sense to introduce a new dedicated default-cache setting just for the cache template tag. The default cache is so named because it is, well, the default.

Being able to specify a cache in the tag syntax makes sense, though.

comment:3 Changed 8 months ago by FunkyBob

Well, yes, and no... I was basing it off the fact staticfiles, by default, will try to use a different cache backend and fall-back to default. I think this is a good pattern for more of Django to follow, allowing people to differentiate if they want.

In our current project we'd like to move all the template fragment caching to a specific backend.

Perhaps a middle ground solution, and have it look for a fixed name [as staticfiles does -- I got confused and thought it had a setting] such as "fragments", and fall back to default otherwise?

comment:4 Changed 8 months ago by carljm

Hmm. In this case, given we apply the tag-argument part of this patch, it could be easily handled in your project with a simple custom wrapper to the cache tag that passes along a different cache name by default. But if we're going to add support directly in Django for redirecting all template-tag caching away from the default cache, I definitely prefer the staticfiles approach to a setting.

comment:5 Changed 8 months ago by FunkyBob

Cool... already done the patch and am working on tests and docs now.

comment:6 Changed 8 months ago by FunkyBob

Code, tests and docs are now ready in that PR

comment:7 Changed 8 months ago by FunkyBob

Timo,

I missed your comment there... yes, I have considered if it should be a hard error or not to specify a cache that doesn't exist.

I can see a case either way, to be honest. But I guess since there's now a fallback approach for "template_fragments" -> "default", when you define one explicitly we probably should go splodey if it's not there.

comment:8 Changed 8 months ago by timo

From IRC, mjtamlyn and I favor carljm's approach of a custom wrapper to the cache tag rather than always defaulting to a "template_fragments" cache.

comment:9 Changed 8 months ago by FunkyBob

*bump*

So, the PR is now ready, adding the following:

1) Allow specifying which cache backend a use of {% cache %} will store in by adding a using="cachename" to its args.

2) Having {% cache %} tags use the cache 'template_fragments' if it's configured, otherwise use 'default'

I see comments against (2), but I'd like to understand why people are against it.

comment:10 Changed 8 months ago by apollo13

@timo: can you say why and if that's a 0 or -1? I think that having an implicit default for the cache with a fallback to 'default' isn't that bad…

comment:11 Changed 8 months ago by mjtamlyn

Just to clarify my viewpoint - I'm happy for there to be a new implicit default, I'm not happy for there to be a new setting. So overall I'm happy with both parts of the patch.

comment:12 Changed 8 months ago by timo

If other people like the implicit default, it's ok with me. My concern is hardcoding a setting that isn't configurable - seems like a small smell, but not too bad as it seems quite unlikely that someone would be using a cache_name of 'template_fragments' for another purpose.

comment:13 Changed 7 months ago by timo

  • Easy pickings unset

comment:14 Changed 6 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 8688f03eef9cf5fd763ba35481ddcff933a60c30:

Fixed #20945 -- Allowed cache tag to use a specific cache.

comment:15 Changed 6 months ago by Tim Graham <timograham@…>

In 382d324ccc0753962ec31ac23a4bde4fb2b9454e:

Added missing newline in docstring; refs #20945.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.