Re: [Django] #34532: Form.default_renderer is ignored in formsets.
Django
----------------------------------+------------------------------------
Reporter: Ryan Burt | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by David Smith):
Here's a scenario.
{{{
class CustomFormRenderer(TemplatesSetting):
...
class MyForm(Form):
name = CharField()
default_renderer = CustomFormRenderer
class CustomFormsetRenderer(Jinja2):
...
MyFormSet = formset_factory(MyForm, renderer=CustomFormsetRenderer)
}}}
Should `MyForm` be rendered with the `CustomFormRenderer` of the
`CustomFormsetRenderer`.
The way that I've thought about this to date is that whatever gets used at
the highest level (formset -> form -> widget) gets passed down to the
lower levels so that the same renderer is used for the whole form/formset.
Does passing your custom renderer `modelformset_factory` work for you?
I think even if no code changes were agreed for this ticket there may be
some doc changes needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
----------------------------------+------------------------------------
Reporter: Ryan Burt | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by Ryan Burt):
Hi,
Passing the renderer to {{{ modelformset_factory }}} does indeed work,
though it needs to be instantiated object rather than passing the class.
I think the concern is that in your scenario, setting {{{ default_renderer
= CustomFormRenderer }}} is still unsupported as the code block that uses
the default_renderer is inaccessible.
This leaves a confusing mix of behaviour for the ModelForm class as the
default_renderer will work when instantiating directly, but not when using
{{{ formset_factory }}}. I agree with the hierarchical intent that lower
level methods shouldn't override supplied variables from their parents.
I think that altering the attrs in {{{ formset_factory }}} from:
{{{
attrs = {
"form": form,
"extra": extra,
"can_order": can_order,
"can_delete": can_delete,
"can_delete_extra": can_delete_extra,
"min_num": min_num,
"max_num": max_num,
"absolute_max": absolute_max,
"validate_min": validate_min,
"validate_max": validate_max,
"renderer": renderer or get_default_renderer(),
}
}}}
To:
{{{
attrs = {
"form": form,
"extra": extra,
"can_order": can_order,
"can_delete": can_delete,
"can_delete_extra": can_delete_extra,
"min_num": min_num,
"max_num": max_num,
"absolute_max": absolute_max,
"validate_min": validate_min,
"validate_max": validate_max,
"renderer": renderer,
}
}}}
Is a non breaking change, as the return is the very next line, which
instantiates the Form class:
{{{
return type(form.__name__ + "FormSet", (formset,), attrs)
}}}
And the renderer variable is not used anywhere prior inside {{{
BaseForm.__init__ }}} :
{{{
def __init__(
self,
data=None,
files=None,
auto_id="id_%s",
prefix=None,
initial=None,
error_class=ErrorList,
label_suffix=None,
empty_permitted=False,
field_order=None,
use_required_attribute=None,
renderer=None,
):
self.is_bound = data is not None or files is not None
self.data = MultiValueDict() if data is None else data
self.files = MultiValueDict() if files is None else files
self.auto_id = auto_id
if prefix is not None:
self.prefix = prefix
self.initial = initial or {}
self.error_class = error_class
# Translators: This is the default suffix added to form field
labels
self.label_suffix = label_suffix if label_suffix is not None else
_(":")
self.empty_permitted = empty_permitted
self._errors = None # Stores the errors after clean() has been
called.
# The base_fields class attribute is the *class-wide* definition
of
# fields. Because a particular *instance* of the class might want
to
# alter self.fields, we create self.fields here by copying
base_fields.
# Instances should always modify self.fields; they should not
modify
# self.base_fields.
self.fields = copy.deepcopy(self.base_fields)
self._bound_fields_cache = {}
self.order_fields(self.field_order if field_order is None else
field_order)
if use_required_attribute is not None:
self.use_required_attribute = use_required_attribute
if self.empty_permitted and self.use_required_attribute:
raise ValueError(
"The empty_permitted and use_required_attribute arguments
may "
"not both be True."
)
# Initialize form renderer. Use a global default if not specified
# either as an argument or as self.default_renderer.
if renderer is None:
if self.default_renderer is None:
renderer = get_default_renderer()
else:
renderer = self.default_renderer
if isinstance(self.default_renderer, type):
renderer = renderer()
self.renderer = renderer
}}}
Essentially, if renderer is none when calling {{{ formset_factory }}}, {{{
get_default_renderer() }}} is still called lower in the api if required,
but still before {{{ self.renderer }}} is ever utilised in the form
widgets. This improves your scenario as you could do something like:
{{{
class CustomFormRendererPrimary(TemplatesSetting):
...
class CustomFormRendererSecondary(DjangoTemplates):
...
class MyForm(Form):
name = CharField()
default_renderer = CustomFormRendererPrimary
class CustomFormsetRenderer(Jinja2):
...
if use_secondary:
MyFormSet = formset_factory(MyForm,
renderer=CustomFormRendererSecondary())
else:
MyFormSet = formset_factory(MyForm)
}}}
Adding the caveat to the docs that {{{ default_renderer }}} through {{{
formset_factory }}} is unsupported and that passing the renderer directly
is the supported method could suffice, as I'm not sure how many projects
are affected by this and haven't figured out to pass renderer directly (or
override {{{ __init__ }}} to set the renderer like I did originally).
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:7>
Django
----------------------------------+------------------------------------
Reporter: Ryan Burt | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by Christopher Cave-Ayland):
I've been looking at this as part of the sprints for DjangoCon EU. I've
put together
[https://github.com/cc-a/django/blob/ticket_34532/tests/forms_tests/tests/test_formsets.py#L1556-L1576
a test] (gist pasted below) that I believe captures the desired behaviour
discussed above.
{{{
def test_form_default_renderer(self):
"""
In the absense of a renderer passed to formset_factory, the
default_renderer
attribute of the Form class should be respected.
"""
from django.forms.renderers import Jinja2
class ChoiceWithDefaultRenderer(Choice):
default_renderer = Jinja2
data = {
"choices-TOTAL_FORMS": "1",
"choices-INITIAL_FORMS": "0",
"choices-MIN_NUM_FORMS": "0",
}
ChoiceFormSet = formset_factory(ChoiceWithDefaultRenderer,
renderer=None)
formset = ChoiceFormSet(data, auto_id=False, prefix="choices")
self.assertIsInstance(
formset.forms[0].renderer,
ChoiceWithDefaultRenderer.default_renderer
)
}}}
This test currently fails but can be made to pass via the above change
suggested via @RyanBurt.
The change unfortunately is breaking however and causes 14 other test
failures. The root cause seems to be that the `renderer` attribute for the
resulting formset class ends up as `None` even though it is expected not
to be.
To progress this ticket I think some more sophisticated changes to
BaseFormSet are required to determine the appropriate renderer to use for
forms. I'll aim to update after some further experimentation.,
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:8>
Django
----------------------------------+------------------------------------
Reporter: Ryan Burt | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by Kees Hink):
My and my colleagues tried to reproduce and illstrate the issue. We did so
in this repository: https://github.com/fourdigits/ticket_34532/
Based on this, we feel that the issue as reported by Ryan Burt has merit:
when we use formset_factory to create a formset, the default should be to
use the form's renderer. \
Changing this behavior, as Christopher Cave-Ayland already tried, would be
welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:9>
Django
Reporter: Ryan Burt | Owner:
| Christopher Cave-Ayland
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Christopher Cave-Ayland):
* owner: nobody => Christopher Cave-Ayland
* status: new => assigned
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/16916
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Burt | Owner:
| Christopher Cave-Ayland
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Marking "Needs" flags based on David's
[https://github.com/django/django/pull/16916#issuecomment-1579506419
comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:11>
Django
Reporter: Ryan Burt | Owner: David Smith
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: default_renderer | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by David Smith):
* owner: Christopher Cave-Ayland => David Smith
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:12>
Django
Reporter: Ryan Burt | Owner: David
| Smith
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Ryan Burt | Owner: David
| Smith
Component: Forms | Version: 4.2
Severity: Normal | Resolution: fixed
Keywords: default_renderer | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"95e4d6b81312fdd9f8ebf3385be1c1331168b5cf" 95e4d6b]:
{{{
#!CommitTicketReference repository=""
revision="95e4d6b81312fdd9f8ebf3385be1c1331168b5cf"
Fixed #34532 -- Made formset_factory() respect Form's default_renderer.
Co-authored-by: David Smith <smi...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34532#comment:14>