[WIP] Fixed #34077 -- Made BoundField renderable. #16247

smithdc1 · 2022-11-02T20:19:06Z

carltongibson

Quick initial comment: this should have a major section at the top of the release notes, with a full usage example, I think. (We wanna tell folks about it! 😜)

Nice work picking this up David 🦄

ngnpope · 2022-11-03T17:41:44Z

django/forms/boundfield.py

+    def as_field(self, template_name=None, context=None, renderer=None):
+        """
+        Render the field including labels, help text and errors.
+        """
+        renderer = renderer or self.form.renderer
+        template = template_name or self.field.template_name
+        context = context or {"field": self}
+        return mark_safe(renderer.render(template, context))


This is very close to django.forms.utils.RenderableMixin.render(), so wondering if we could define:

# django/form/utils.py class RenderableFieldMixin(RenderableMixin): def as_field(self): return self.render(self.template_name_field)

Or something like that. And then have the following here:

class BoundField(RenderableFieldMixin): ... def get_context(self): return {"field": self}

We'd also need to set self.renderer in BoundField.__init__(). Then we could potentially ditch the @html_safe and __str__(), but we'd have to see how we can make that behave still as currently it relies on .as_widget() which looks a bit different. I think we could overcome that though with some thought... It might be that we then later want to revisit what widgets are doing and see if we need something like a RenderableWidgetMixin too. It'd be really good to support a consistent API for all of this, especially for long term maintenance 🤔

This highlighted a nice bug I'd likely introduced. Hopefully we'll have one test failure showing this.

We have a circular series of renderables as the form contains its boundfield and the boundfield has a reference back to the form.

While I need to spend more time looking at this I'd also appreciate ideas.

ngnpope · 2022-11-03T17:43:10Z

docs/ref/forms/api.txt

@@ -1243,6 +1243,14 @@ Attributes of ``BoundField``
 Methods of ``BoundField``
 -------------------------

+.. method:: BoundField.as_field(template_name=None, context=None, renderer=None)


Yeah, looking at this, this is the signature of RenderableMixin.render(), not the various .as_*() methods.
Would definitely be nice to align this better.

carltongibson · 2022-11-10T10:21:06Z

Hey @smithdc1 — Just looking at the discussion here. Can I ask you to ping me when you're ready for a review. Thanks. 🎁

smithdc1 · 2022-11-14T20:43:42Z

@carltongibson -- Can I ask you to put this on your list please?

On the circular rendering issue, my current thinking is that it's actually ok for now. The original issue was recurively calling __str__ gave rise to a circular context. This patch doesn't change BoundField.__str__ and therefore I don't belive this patch as is should be causing that regession. I think the error is that the test incorrectly calls render().

I've changed the test to call __str__() instead. I believe this to still be a regression test for https://code.djangoproject.com/ticket/33134 but also allows the tests to pass with this patch. 🤞

The for now reference is that if we ever did want to change BoundField.__str__() to render the field then we'd encounter that regression test.

ngnpope

Thanks David. This is a good next step, but I think we need to think about extracting the field rendering for more than just the div.html. Really .as_field() should be .as_div() and we should add the equivalent for the other tags.

I'll try and have a think through the mess with .as_widget() and .as_hidden() in .__str__(). It feels pretty clunky and looks sort of outdated w.r.t. moving stuff to template rendering. Biggest pain is that they're documented API.

ngnpope · 2022-11-15T10:21:30Z

django/forms/utils.py

+            "Subclasses of RenderableFieldMixin must provide an as_widget() method."
+        )
+
+    def __str__(self):


Probably worth a comment to state that we override this here for backward compatibility, i.e. we can't rely on the default implementation from the parent that uses self.render().

ngnpope · 2022-11-15T10:23:43Z

django/forms/utils.py

+    # TODO -- I'm not sure how best to structure this as __str__ on boundfield does
+    # something different to RenderableMixin.


I guess this comment is really related to __str__() below. I think we can start out with the comment I've mentioned.

I'll try to have a think if there is some backward-compatible way we can improve this. But either way, even if it's not an entirely clean fit we're at least coercing this into a more clear pattern.

ngnpope · 2022-11-15T10:35:57Z

django/forms/templates/django/forms/field.html

@@ -0,0 +1,10 @@
+{% if field.use_fieldset %}


I notice that we're extracting this from the django/forms/div.html template.. But what about the others?

Maybe this should be django/forms/fields/div.html and we should also extract the equivalent from p.html, table.html and ul.html. Shouldn't we have .as_div(), .as_table(), .as_p(), and .as_ul() like at the form level instead of .as_field()?

It certainly feels like this should mirror what happens at the form level as the field should follow the same templating structure as the form to avoid incorrect output.

tests/forms_tests/templatetags/tags.py

ngnpope · 2022-11-15T10:45:10Z

django/forms/utils.py

+        if self.field.show_hidden_initial:
+            return self.as_widget() + self.as_hidden(only_initial=True)
+        return self.as_widget()


Hmm. The more I look into the these and some of the existing helpers, the more I feel that some stuff should be cleaned up and ripped out. I'll need some time to have a play though.

This mornings thought was I wonder if the first step here would be to see if we can make Widget inherit from RenderableMixin or similar.

I don't think we can currently call str() on a widget to get it to render. That could be nice in a template. 🤔

{{ field.label }} {{ field.help_text }} {{ field.widget }} ...

Thinking out loud here!

smithdc1 · 2022-11-15T20:21:21Z

Thank you for you review and comments as always.

Just a couple of initial comments.

we should add the equivalent for the other tags.

We don't recommend the other methods. We stopped short of removing them (that was the suggestion by the accessibility team) due to backward compatibility concerns. So I wasn't sure on adding additional support for them.

Maybe more importantly I'm not sure there's that much benefit. How many folk are using exactly the standard layout for those methods and would benefit (e.g im not sure how altering the table layout would help)? As soon as you have something slightly different the advise would be to create a custom template and use that instead? Assign it in the renderer or field or ...

Biggest pain is that they're documented API.

It's also been like this for some time.
https://github.com/django/django/blame/4d596a1f6443eaf5d18d70a18aaac25030c7fc81/django/newforms/__init__.py#L829

I had assumed that was too much to be changing at this stage.

carltongibson · 2022-11-23T09:37:31Z

We don't recommend the other methods.

Yes... — if we were starting from scratch now, we wouldn't have the as_*() methods on form: rather you'd just set the template and output as a string (str(form))

So here… — if we were starting from scratch now, we'd do the same for BoundField, except we can't because __str__() is already taken. Thus, we need to add the new as_field() (still thinking about the name there) in order to allow folks to output the errors/help-text/widget group all in one go. (The docs should be clear that str(bf) just output the widget, where bf.as_field() outputs the collected parts.)

I don't think we need (or, for the a11y concerns, want) to add as_p() &co like templates. Let's just provide one that. The div templates are becoming the default, let's go with that. (Any issues there? 🤔)

On the circular rendering issue...

👀

ngnpope · 2022-11-24T09:25:39Z

Yes... — if we were starting from scratch now, we wouldn't have the as_*() methods on form: rather you'd just set the template and output as a string (str(form))

Ah, if only, eh? 🙂

I don't think we need (or, for the a11y concerns, want) to add as_p() &co like templates. Let's just provide one that. The div templates are becoming the default, let's go with that. (Any issues there? thinking)

If we're to do this - only focus on improving the .as_div() case - I would like to see a stronger admonition under the docs for .as_p(), .as_table(), and .as_ul(). It should state that we're no longer developing improvements around those helpers, that they're poor for accessibility reasons, and that we recommend migrating over to using .as_div() instead. We should possibly even say that they're soft-deprecated. Currently we only mention this under .as_div(), but it doesn't feel strong enough (to me) to warn people away from them.

Not splitting out those cases and advertising this split-out field template as a new feature may be confusing to users of those non-recommended methods.

carltongibson · 2022-11-24T10:48:10Z

Hey @ngnpope. 👍

We already moved the as_* methods to the bottom of Outputting forms as HTML, in the subsection here: https://docs.djangoproject.com/en/4.1/ref/forms/api/#output-styles, that you linked. This was an attempt to de-emphasise them. We could move the note in the as_div() bit to the top of that subsection, and add another sentence saying, ≈_"In general the {{ form }} usage shown above is preferred. These helpers largely remain for backwards compatibility reasons."_

smithdc1 · 2022-12-14T14:02:23Z

I'll come back to this after the feature freeze. 🎅🎄

I've spent a bit of time looking at the comments and it seems to unravel and get more complex the more I think through it. I'm not sure unpicking the current str behaviour is that straight forward. 🤔

carltongibson · 2022-12-14T14:38:36Z

I'll come back to this after the feature freeze. 🎅🎄

Hey @smithdc1 — OK. Always wise 💝 — Overall, I think this is a (really) good feature to have, but there's no urgency. 5.0 will be more than perfect. 🎁

smithdc1 · 2023-02-08T09:51:15Z

Hey @carltongibson 👋

Just to say, I'm planning on coming back to this next. I've got one eye on trying to get this in before you move onto new adventures. 😀

carltongibson · 2023-02-08T09:56:30Z

Cool. Yes. Let's get it in. It's the icing on the cake for the forms changes over the last few releases.

(I didn't get to think about it but, did we consider the template tag approach? Do we favour the method on BoundField?)

smithdc1 · 2023-02-10T20:44:28Z

(I didn't get to think about it but, did we consider the template tag approach? Do we favour the method on BoundField?)

I think I originally suggested this as an option on the ticket. I've come round to the idea that it is not the best option here. All of the other ways of rendering a form/field/widget are a method, usually one of __str__(), render() or one of the as_*() methods.

To suddenly have a template tag to render a field seems like an odd choice.

I think the most difficult question we have to answer is what do we do about calling __str__ on a BoundField, and the fact that it returns the fields widget. As Nick pointed out it's not just __str__ there's also .as_widget() and .as_hidden() so it's quite a lot to try and unpick. It's also been that way ~forever. Do we try and deprecate that so we can clean up the rest of the code?

We could move the note in the as_div() bit to the top of that subsection ...

I'll think I'll start here, likely as another PR so we can get that in.

smithdc1 · 2023-02-17T19:11:58Z

docs/releases/5.0.txt

@@ -37,6 +37,70 @@ compatible with Django 5.0.
 What's new in Django 5.0
 ========================

+BoundField becomes renderable


This needs a better title.

carltongibson · 2023-03-01T10:19:37Z

Closing in favour of #16574

carltongibson reviewed Nov 2, 2022

View reviewed changes

ngnpope reviewed Nov 3, 2022

View reviewed changes

ngnpope reviewed Nov 15, 2022

View reviewed changes

carltongibson self-requested a review November 23, 2022 09:24

smithdc1 force-pushed the boundfield branch from 7fe9278 to 81644b5 Compare January 28, 2023 19:00

smithdc1 mentioned this pull request Feb 12, 2023

Refs #16247 -- Updated docs to use default <div> style and refactored docs to have less emphasis on as_* methods. #16545

Merged

Fixed #34077 -- Made BoundField renderable.

728cdda

smithdc1 force-pushed the boundfield branch from 81644b5 to 728cdda Compare February 17, 2023 19:06

smithdc1 commented Feb 17, 2023

View reviewed changes

Docs edits

4912625

smithdc1 mentioned this pull request Feb 19, 2023

Fixed #34077 -- Added Renderable BoundField and BoundWidgets #16574

Closed

carltongibson closed this Mar 1, 2023

smithdc1 deleted the boundfield branch November 27, 2023 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fixed #34077 -- Made BoundField renderable. #16247

[WIP] Fixed #34077 -- Made BoundField renderable. #16247

smithdc1 commented Nov 2, 2022

carltongibson left a comment

ngnpope Nov 3, 2022

smithdc1 Nov 7, 2022

ngnpope Nov 3, 2022

carltongibson commented Nov 10, 2022

smithdc1 commented Nov 14, 2022

ngnpope left a comment

ngnpope Nov 15, 2022

ngnpope Nov 15, 2022

ngnpope Nov 15, 2022

ngnpope Nov 15, 2022

smithdc1 Nov 16, 2022

smithdc1 commented Nov 15, 2022

carltongibson commented Nov 23, 2022

ngnpope commented Nov 24, 2022

carltongibson commented Nov 24, 2022 •

edited

smithdc1 commented Dec 14, 2022

carltongibson commented Dec 14, 2022

smithdc1 commented Feb 8, 2023

carltongibson commented Feb 8, 2023

smithdc1 commented Feb 10, 2023

smithdc1 Feb 17, 2023

carltongibson commented Mar 1, 2023

		# TODO -- I'm not sure how best to structure this as __str__ on boundfield does
		# something different to RenderableMixin.

[WIP] Fixed #34077 -- Made BoundField renderable. #16247

[WIP] Fixed #34077 -- Made BoundField renderable. #16247

Conversation

smithdc1 commented Nov 2, 2022

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carltongibson commented Nov 10, 2022

smithdc1 commented Nov 14, 2022

ngnpope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smithdc1 commented Nov 15, 2022

carltongibson commented Nov 23, 2022

ngnpope commented Nov 24, 2022

carltongibson commented Nov 24, 2022 • edited

smithdc1 commented Dec 14, 2022

carltongibson commented Dec 14, 2022

smithdc1 commented Feb 8, 2023

carltongibson commented Feb 8, 2023

smithdc1 commented Feb 10, 2023

Choose a reason for hiding this comment

carltongibson commented Mar 1, 2023

carltongibson commented Nov 24, 2022 •

edited