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

Fixed #15667 -- Added template-based widget rendering. #6498

Merged
merged 1 commit into from Dec 27, 2016

Conversation

Projects
None yet
7 participants
@timgraham

This comment has been minimized.

Copy link
Member Author

timgraham commented Apr 30, 2016

As currently implemented, updating a project for this change requires either installing Jinja2 or configuring a DjangoTemplates with APP_DIRS: True. Do you think the lack of graceful backwards compatibility is okay?

@MoritzS, if you'd like to contribute any improvements to the jinja2 templates as suggested in your comment on the original PR, that would be welcome.

@MoritzS

This comment has been minimized.

Copy link
Contributor

MoritzS commented May 1, 2016

I looked over the jinja2 templates again: To really make them look nicer, the trim_blocks and lstrip_blocks options to the environment are needed so I guess they are fine as is.

@timgraham timgraham force-pushed the 15667 branch 2 times, most recently from b34ce36 to 22af1cd May 2, 2016

@charettes

This comment has been minimized.

Copy link
Member

charettes commented May 9, 2016

@timgraham, about the 'django.forms' in INSTALLED_APPS issue. I think requiring a special loader that knows where to load the widget templates from would make more sense. I'll try to come up with a POC today. I'm thinking of a django.forms.templates.loaders.WidgetTemplateLoader that would need to be added to the TEMPLATES[index]['OPTIONS']['loaders']list.

@timgraham timgraham force-pushed the 15667 branch from 22af1cd to 83e0138 May 9, 2016

if templates_configured():
return

if not jinja2:

This comment has been minimized.

@carljm

carljm May 9, 2016

Member

this logic should maybe just go inside the default_engine static method?

@timgraham timgraham force-pushed the 15667 branch 2 times, most recently from 1c26943 to 75e9dd4 May 11, 2016

@carljm

This comment has been minimized.

Copy link
Member

carljm commented May 12, 2016

I'm still planning to push the changes from carljm@7d734cf plus tests (and the template-not-found chaining fix that @prestontimmons proposed), but I've been a bit distracted with DEP 5 stuff and haven't gotten to it yet. Hopefully tonight.

@carljm

This comment has been minimized.

Copy link
Member

carljm commented May 13, 2016

@timgraham I did just notice one html-escaping test failing in forms_tests when Jinja2 is installed. Looks like a simple issue of " vs ". Otherwise I've got nothing in mind besides the docs checklist.

Re the public renderer API, although for TemplateRenderer we document both render and get_template (since the latter is generally more useful to override), I think we should clarify somewhere that actually render is the only method that Django calls on your renderer, and the only one that has to be there.

@timgraham timgraham force-pushed the 15667 branch 3 times, most recently from f4c3164 to f99b933 May 14, 2016

@timgraham timgraham changed the title [WIP] Fixed #15667 -- Added template-based widget rendering. Fixed #15667 -- Added template-based widget rendering. May 14, 2016

@timgraham timgraham force-pushed the 15667 branch 3 times, most recently from 25cdb28 to a9464fd May 29, 2016

@timgraham timgraham force-pushed the 15667 branch 2 times, most recently from f8e1818 to 0ae75ca Jun 9, 2016

timgraham referenced this pull request Jun 17, 2016

Refs #15667 -- Removed choices argument from some RendererMixin methods.
RendererMixin will soon be removed but this removal and the corresponding
test changes stand on their own.
@@ -0,0 +1,61 @@
import os

This comment has been minimized.

@timgraham

timgraham Dec 12, 2016

Author Member

@prestontimmons, Just to be sure that the module structure isn't needlessly complicated, I was wondering if you envisioned other renders in other files in this module at some point?

This comment has been minimized.

@prestontimmons

prestontimmons Dec 12, 2016

Contributor

I think at one point I had it split out into renderers/django.py and renderers/jinja2.py. I think it makes sense to just be renderers.py.

@timgraham

This comment has been minimized.

Copy link
Member Author

timgraham commented Dec 12, 2016

I think this is ready for review.

Not sure if the new SEARCH_APP_DIRS_BEFORE_DIRS key for TEMPLATES is a good idea or not (if so, doc updates may be needed). The main reason I added it is because It seems cleanest if django/forms renderers aren't overriding or reimplementing private parts of template engines. \cc @aaugustin

@carljm
Copy link
Member

carljm left a comment

Looks good, modulo a few comments! I didn't re-review all the widget stuff, just the core renderers code and its docs.

I don't mind adding post-app-dirs template dirs as a feature of template backends, but I think it would be both simpler to understand and implement, and actually more powerful, to introduce POST_APP_DIRS as a separate list of dirs, rather than a boolean SEARCH_APP_DIRS_BEFORE_DIRS.

})


class ProjectTemplateRenderer(BaseTemplateRenderer):

This comment has been minimized.

@carljm

carljm Dec 12, 2016

Member

This renderer probably also deserves a docstring?

@@ -24,6 +24,9 @@ def __init__(self, params):
self.name = params.pop('NAME')
self.dirs = list(params.pop('DIRS'))
self.app_dirs = bool(params.pop('APP_DIRS'))
# This option ended up working only for Jinja2 because DjangoTemplates

This comment has been minimized.

@carljm

carljm Dec 12, 2016

Member

This comment seems confusing/misleading. AFAICT this option works and is used for both Jinja2 and DTL engines, although in different ways/locations; DTL just passes it down to the Engine.

The low-level render API
========================

Widget templates are stored in the ``django/forms/widgets`` path. A project

This comment has been minimized.

@carljm

carljm Dec 12, 2016

Member

The second sentence is slightly misleading. "A project" cannot by default override a widget template (unless the default form renderer is changed to ProjectTemplateRenderer), though an installed app can do so. I think just changing "A project" to "An installed app" would preserve the main point while being technically accurate.

This comment has been minimized.

@timgraham

timgraham Dec 13, 2016

Author Member

Correct, although I'm concerned that it gives the idea that third-party apps should be overriding templates to customize things when that's really not appropriate since only the template in the earliest installed app will be found. Do you agree that guideline?

This comment has been minimized.

@carljm

carljm Dec 13, 2016

Member

Yeah, I agree that third-party apps in most cases shouldn't be overriding form widgets (though I suppose I could see some exceptions for e.g. something like a django-bootstrap-forms app, if it clearly advertises that this is what it does).

Maybe it'd be possible to rephrase this sentence in a way that doesn't have to mention either "project" or "app", since the real point here is just that form widgets can be overridden. Maybe passive voice? Or do we use second person in the docs? "You can provide a custom template for..."

#. Adding the built-in widgets templates directory (``django/forms/templates``
or ``django/forms/jinja2``) in ``DIRS`` of one of your template engines.

You've chosen to take full control, so it's your responsibility to make

This comment has been minimized.

@carljm

carljm Dec 12, 2016

Member

missing "sure" after "make"


.. module:: django.forms.renderers.templates

.. class:: BaseTemplateRenderer

This comment has been minimized.

@carljm

carljm Dec 12, 2016

Member

I'm not sure that we should document BaseTemplateRenderer or EngineRendererMixin; they seem like internal implementation details to me. I think any hypothetical value they'd provide to the implementor of a custom renderer (which is minor, they don't do very much) is outweighed by the added "ravioli code" complexity introduced by inheriting from built-in mixins and base classes. If the built-in renderers don't suit, the implementor of a custom renderer is likely to need to modify the code provided by these classes anyway, and I think we do them a better service by encouraging them to just implement what they need directly.

Documenting these also reduces our refactoring flexibility in the future.

@timgraham

This comment has been minimized.

Copy link
Member Author

timgraham commented Dec 12, 2016

POST_APP_DIRS seems okay, although the implementation for DjangoTemplates seems a bit odd. The filesystem loader currently works by looking at self.engine.dirs, so the first approach that comes to mind is to add a filesystem loader subclass that looks at self.engine.post_app_dirs instead.

@carljm

This comment has been minimized.

Copy link
Member

carljm commented Dec 12, 2016

The current implementation of the filesystem loader (looking at self.engine.dirs) evolved naturally from its origins looking directly at settings.TEMPLATE_DIRS, but a more flexible implementation would just take dirs as an extra loader parameter (we have precedent for loaders that take additional args: the cached loader). That allows inserting potentially two instances of the filesystem loader into the loaders chain, looking at two different lists of dirs, without needing a subclass.

Can of course make the new parameter optional and fall back to self.engine.dirs for back-compat and convenience in the common case.

@timgraham timgraham force-pushed the 15667 branch 5 times, most recently from 37ae6c9 to 149eddf Dec 13, 2016

@carljm

carljm approved these changes Dec 19, 2016

Copy link
Member

carljm left a comment

Looks good to me! As before, I reviewed the core renderers code, tests, and docs; I didn't look over all the widget changes in detail. I don't think those have changed recently?

from django.utils.functional import cached_property
from django.utils.module_loading import import_string

from . import __file__ as forms_file

This comment has been minimized.

@carljm

carljm Dec 19, 2016

Member

What is the benefit of this import over just using our own __file__ directly? If I'm not mistaken, one would result in .../django/forms/__init__.py and the other would result in .../django/forms/renderers.py, which is irrelevant since either way we're about to call os.path.dirname(...) on it. What am I missing?


To use this backend, all the widgets in your project and its third-party apps
must have Jinja2 templates. You can't use this renderer, for example, if you're
using :mod:`django.contrib.admin` because Jinja2 templates aren't included for

This comment has been minimized.

@carljm

carljm Dec 19, 2016

Member

Is it worth adding "(unless you provide Jinja templates for all the admin widgets yourself)" here?

os.path.join(
upath(os.path.dirname(__file__)),
'..',
getattr(self, 'expected_widget_dir', 'templates') + '/forms_tests/custom_widget.html',

This comment has been minimized.

@carljm

carljm Dec 19, 2016

Member

Why not just set expected_widget_dir = 'templates' as a class attr on SharedTests and avoid the need to do this getattr twice?

@apollo13

This comment has been minimized.

Copy link
Member

apollo13 commented Dec 20, 2016

So, bare with me, I just got another completely stupid idea. But before I come to it, lets take a step back: The current renderer system is all nice and well but it cries for user problems (speaking with my IRC support hat on) -- ie why is the Django form renderer not picking the default template settings like libraries. It would be nice if we could just reuse an existing template engine, no?

So what I came up with is the following: Only ship a django.forms.renderers.ProjectTemplateRender, this renderer will use the (new) setting FORM_RENDERER_ENGINE (which defaults to default) to choose a template engine from settings.py, from which it will copy the settings/options from and initialize a new engine accordingly while adding post_apps_dirs. This keeps the whole flexibility we currently have, while adding more convenience to it.

A second option which follows naturally is that we could even consider dropping the renderers completely and only use FORM_RENDERER_ENGINE as public setting to guide which engine gets used. After all the API for renderers consists of two methods: get_template which just passes on to the engine to select a template and render which might allow someone to hook in extra default context based on the request or so -- at which point a natural extension would be to provide a hook on the engine if we wanted that. Even without hooks, it seems appropriate to subclass engines if you want extra behavior as opposed to a new renderer class completely.

With the second option we would then achieve the following:

  • The already configured template engines are used, yay
  • Less user confusion about missing tags/filters/context_processors, double yay
  • Less additional classes, less extra abstraction for no real gain

EDIT:// And best of all: If we really need the renderers in the future, we can just readd them ;)

@carljm

This comment has been minimized.

Copy link
Member

carljm commented Dec 20, 2016

@apollo13 Most of the ideas in your proposal already came up at one time or another in the long discussion of the options here :-) A few problems:

  1. We have zero guarantees about the capabilities of the default template engine someone has configured, and we need to provide backwards-compatible form rendering, including e.g. finding admin widget templates. Someone may have a Jinja2 default engine; that right there would break admin widget rendering.

  2. There is a big difference between the current ProjectTemplateRenderer that uses get_template (that is, will check all engines and use them exactly as-is) and your proposed one that picks only one specific engine (and then copies its settings and automatically adds the core form templates dir, with I guess in your proposal no way to turn that off?). I definitely want to have the former available; ultimately for most projects I think that is the simplest destination, so I won't be happy if only the latter is available.

  3. Copying an existing engine's settings and magically adding the built-in form templates dir is ugly magic :-)

I think you may be right, though, that people will find the default renderer (as currently coded) to be a source of confusion, because it's close to the same as a typical TEMPLATES setup, but not quite.

Also, I just realized that allowing app overrides in the default renderer is a potential backwards-compatibility issue; people may already have an installed app that unintentionally/accidentally "overrides" a template path we are using for a form widget. Not likely, maybe, but possible.

All of which makes me think that @timgraham was right about not allowing apps to override form templates in the default renderer. In that case IMO we should treat the default renderer as basically a back-compat shim, with the intent being that most people will eventually be best off switching to ProjectTemplateRenderer (and I think we should even consider switching the startproject template to use ProjectTemplateRenderer, though we could also leave that for separate consideration).

@prestontimmons

This comment has been minimized.

Copy link
Contributor

prestontimmons commented Dec 20, 2016

All of which makes me think that @timgraham was right about not allowing apps to override form templates in the default renderer.

The wrench in this plan is the admin/gis widgets. They won't be discoverable by default if app directories aren't searched.

@carljm

This comment has been minimized.

Copy link
Member

carljm commented Dec 20, 2016

@apollo13

This comment has been minimized.

Copy link
Member

apollo13 commented Dec 20, 2016

@carljm I feared as much, I just recently read the latest thread on the ML to get a bit of an overview, but on the other hand I also wanted to provide a fresh new perspective without being influenced too much by the decision process so far to make it easier for my mind to come up with new solutions (I realize that this adds extra work for you, which is probably not fair, so I'll just try to answer your last post and if you all think I am still on the wrong track, I'll leave it be). Sorry in advance for any extra work I cause(d).

Since you've split your arguments so nicely I'll try to answer them one by one -- they don't seem to overlap that much anyways

  1. Your argument here seems to be based on backwards compat solely and therefore the need to render via a Django engine by default. Fair enough. So lets set FORM_RENDER_ENGINE to a sentinel ala NOT_CONFIGURE=object() in global_settings. If this sentinel is set, we can use a new django engine and show a deprecation warning that the user will have to explicitly configure that setting.

  2. That is a good argument which I did only consider after sending my post and going away from the computer. What ProjectTemplateRenderer in it's current form allows for is mainly rendering the admin (and potentially other apps) with Django templates and the rest with (faster?) Jinja templates. This is certainly good and I want to keep that! We could use FORM_RENDER_ENGINE=None to achieve said effect. I realize that None is a stupid option for that and another (maybe uppercased string) value might be better suited, but lets just (for the sake of argument) accept None. I fully agree that in the long run ProjectTemplateRenderer is the simplest solution, but this will not be the default for now, so people setting FORM_RENDER_ENGINE=None can add django.forms at the last position to INSTALLED_APPS too.

  3. Is it really that bad? How engines are used is imo an implementation detail, nothing says that there will ever be only one instantiation of said engine. Also, something to consider: Given my approach to backwards compatibility (ie NOT_CONFIGURED by default) we can force users to add django.forms when changing FORM_RENDER_ENGINE via a system check, then we can completely drop POST_APP_DIRS after a deprecation period and reuse the real engines.

I think you may be right, though, that people will find the default renderer (as currently coded) to be a source of confusion, because it's close to the same as a typical TEMPLATES setup, but not quite.

And please lets not sweep that under the rock, the first question in IRC I (and everyone else in there) will have to answer for weeks will be:

  • I have a nice context_processor where I specify the theme (color) of my material design template, why is the variable not in the templates for widgets, after all it is in the same template directory.
  • I am already using django-sniplates via libraries in the TEMPLATE config, as migration step I want to reuse that for the widgets, why are they not getting picked up.

that unintentionally/accidentally "overrides" a template path we are using for a form widget. Not likely, maybe, but possible.

Yeah, screw that :D (All our approaches to backwards compat have usually been like: "it works in 99% of the cases, but if you have that one obscure config, you have to change a little bit, sorry")

All of which makes me think that @timgraham was right about not allowing apps to override form templates in the default renderer. In that case IMO we should treat the default renderer as basically a back-compat shim

This is a very good question. And honestly something we need to think more about, cause currently if you start overriding widgets, it will affect the admin too, which is not something you want (since the admin doesn't override all widgets but only some) and most likely should stay in the design we provided. If we do not allow those overrides we can add the django.forms template directory as last entry in DIRS and get rid of POST_APP_TEMPLATES completely again. If the users then switch to the ProjectTemplateRender they can put django.forms into any position that suits them. The only downside then would be that you have no way of changing the widgets for lets say everything but the admin (Or any other app for that matter). What I think the admin (and maybe even other 3rd party apps providing their own UI should be doing is maybe something like):

  • either set template (hypothetical, didn't look if that already works) on the widget to a list ala ['admin/widgets/textedit.html', 'django/forms/textedit.html'] like we have with normal templates. This way if you override the default templates everywhere you can also provide a second location (in this case admin/ where the user can copy the default templates so the admin stays the same).
  • Probably provide a templatetag ala {% set_form_renderer form 'renderer' %} so template designer can actually change that (within configured boundaries). But that is just thinking far ahead.
@timgraham

This comment has been minimized.

Copy link
Member Author

timgraham commented Dec 20, 2016

On the last point, the admin doesn't override any widgets. All its custom widgets are namespaced such as template_name = 'admin/widgets/radio.html'.

@carljm

This comment has been minimized.

Copy link
Member

carljm commented Dec 20, 2016

@apollo13 No worries, taking a look with a fresh mindset is good :-)

I am not a fan of the FORM_RENDERER_ENGINE setting. Once you have to default to a bare-bones engine and make people manually update the setting (which you have to, in order to provide reliable back-compat), I don't think it has any advantage over the current approach (start with a renderer that uses a bare-bones default engine, and suggest that people update to ProjectTemplateRenderer). And the latter results in a better and more flexible end-state (use of all of TEMPLATES via get_template, rather than having to pick one specific engine).

I agree with your support concern, but we've been around and around and around this issue: the only way to fully address it would be to somehow use full TEMPLATES automatically, while auto-adding the core forms dir, and there's simply no clean way to do this that is definitely backwards-compatible with the wide variety of possible TEMPLATES setups (including the possible setup that a project currently doesn't use Django template backends at all and has an empty TEMPLATES! We need to provide reliable forms back-compat even for this case) and that doesn't introduce bad coupling between forms and templates.

I think the only feasible option for reliable forms backward-compatibility is what this PR does: default to a simple template engine we construct ourselves that we are sure can definitely load the built-in and contrib-app widget templates. And the best we can do to address the support concern that may arise once people start to use custom widgets and wonder why their TEMPLATES config doesn't apply, is to clearly document that if you're creating custom widgets that need custom template features, you should upgrade to ProjectTemplateRenderer. (And we should update startproject to use ProjectTemplateRenderer, so this support concern is a temporary thing during the upgrade timeframe only.)

I'm really fine either way on the question of whether we use POST_APP_DIRS and allow apps to override built-in widget templates. It's a very minor back-compat concern to allow it, and it requires a slightly kludgy new template-backends feature; and either way I think we should be pushing people towards ProjectTemplateRenderer anyway, in which case allowing overrides by default isn't that important.

@apollo13

This comment has been minimized.

Copy link
Member

apollo13 commented Dec 20, 2016

Mhm, guess I'll rest my case -- If we are going to promote the usage of ProjectTemplateRenderer (which I think we should), we probably should bite the dust and get rid of POST_APP_DIRS and in the same breath of the jinja renderer -- ie provide the Django renderer really only as backwards compat shim.

@timgraham timgraham force-pushed the 15667 branch 4 times, most recently from 5a3fc9e to 8c581a4 Dec 20, 2016

@carljm

carljm approved these changes Dec 23, 2016

Copy link
Member

carljm left a comment

Some doc nits, otherwise lgtm.


The form rendering process can be customized at several levels:

* Widgets can specify custom template names.
* Forms and widgets can specify custom renderer classes.
* Built-in templates can be overridden within a project or application.
* A widget's template can be overridden by a project. (Reusable application's

This comment has been minimized.

@carljm

carljm Dec 23, 2016

Member

No apostrophe in applications


.. versionadded:: 1.11

In older versions, widget are rendered using Python. All APIs described

This comment has been minimized.

@carljm

carljm Dec 23, 2016

Member

widgets (plural)

setting. It defaults to
``'``:class:`django.forms.renderers.DjangoTemplates`\ ``'``.

You can also provide a custom render by setting the

This comment has been minimized.

@carljm

carljm Dec 23, 2016

Member

renderer

It uses :func:`~django.template.loader.get_template` to find widget
templates based on what's configured in the :setting:`TEMPLATES` setting.

Using the renderer along with the built-in widget templates requires either:

This comment has been minimized.

@carljm

carljm Dec 23, 2016

Member

Using this renderer

Use one of the :ref:`built-in template form renderers
<built-in-template-form-renderers>` or implement your own. Custom renderers
must implement a ``render(template_name, context, request=None)`` method. It
should return a :class:`~django.template.Template` object or raise

This comment has been minimized.

@carljm

carljm Dec 23, 2016

Member

It returns the rendered template as a string, it doesn't return a Template object.


* Widgets can specify custom template names.
* Forms and widgets can specify custom renderer classes.
* A widget's template can be overridden by a project. (Reusable application's

This comment has been minimized.

@carljm

carljm Dec 23, 2016

Member

No apostrophe in applications

@timgraham timgraham force-pushed the 15667 branch 2 times, most recently from 1a96b54 to d88281a Dec 23, 2016

Fixed #15667 -- Added template-based widget rendering.
Thanks Carl Meyer and Tim Graham for contributing to the patch.

@timgraham timgraham merged commit b52c730 into master Dec 27, 2016

6 checks passed

default
Details
docs Build #13879 succeeded in 2 min 6 sec
Details
flake8 Build #14019 succeeded in 30 sec
Details
isort Build #14023 succeeded in 15 sec
Details
javascript Build #10377 succeeded in 22 sec
Details
windows Build #6138 succeeded in 12 min
Details

@timgraham timgraham deleted the 15667 branch Dec 27, 2016

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