Opened 4 years ago

Closed 3 years ago

Last modified 5 months ago

#31026 closed Cleanup/optimization (fixed)

Switch form rendering to template engine

Reported by: Johannes Maron Owned by: David Smith
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

We recently switched widgets to use the template engine and template that can be overwritten.
That is currently not the case for forms themselves. More precisely django.forms.BaseForm.as_table and it's siblings.
They use string modulation, that they pass on to the _html_output, which generates the context.

I would propose to change the behavior the following way:
Have _html_output produce a context and don't actually perform a render.
have each as_XYZ method get the context and perform a template based render.

This allows to overwrite or add a new as_XY method and inject something into the context or change the template.

This is also an opportunity to simplify the code. Since templates to render attributes already exist.

Change History (36)

comment:1 by Johannes Maron, 4 years ago

Has patch: set

I drafted something, to add more context, to what I am proposing: https://github.com/django/django/pull/12133/files

comment:2 by Simon Charette, 4 years ago

Triage Stage: UnreviewedAccepted

I think that it makes sense. It'd be great if you could do a bit of research on #15667 to figure if this was simply overlooked or if there was a reason for not doing it in the first place.

I think there's also a ticket out there to allow Form.__str__() to use as_p or as_ul instead of as_table by default. If we were using template based rendering we could simply define a django/forms/default.html that {% include "django/forms/table.html" %}, use it in .__str__() and document that the former must be overridden to achieve the desired behaviour.

Version 0, edited 4 years ago by Simon Charette (next)

comment:3 by Johannes Maron, 4 years ago

Yes, I'll take some time next week to do some research.
The default idea, is great. That would make it even more versatile, love it!

comment:4 by Johannes Maron, 4 years ago

With that change I'd also like to drop [error_class]https://docs.djangoproject.com/en/2.2/ref/forms/api/#customizing-the-error-list-format support, since this would be a template too. However, this is a documented API, though not widely used. I will work around it for now, but I'd really like to remove it.

comment:5 by Johannes Maron, 4 years ago

Ok, I have a working patch now. It does not change tests, or removes much code to prove its backwards compatible.
However, I'd like to introduce some changes, to improve the resulting architecture and output.

  1. Change the position of hidden inputs. Currently, they are injected at a certain index. I'd like to simply amend them. It would greatly simplify the templates. Since they are hidden, it shouldn't change browser rendering.
  2. I'd like to deprecate the now unused django.forms.BaseForm._html_output. Yes, it is private, but we have tests running against it and it's referenced in some tickets. This gives me the impression is more commonly used than we'd like.
  3. I'd like to deprecate django.forms.boundfield.BoundField.label_tag. The label can be generated with a template with a simple include. I believe this to be the better implementation.
  4. Errors I would not change. I am not 100% happy with the design, however, it is based on templates now, which makes adding changes simple.

I haven't written any documentation yet, this will be another monster task. I'd like to get some feedback on the code side first, to avoid back and forth.

comment:6 by Johannes Maron, 4 years ago

Patch needs improvement: set

comment:7 by Johannes Maron, 4 years ago

Patch needs improvement: unset

comment:8 by Johannes Maron, 4 years ago

Needs documentation: set

comment:9 by Johannes Maron, 4 years ago

Patch needs improvement: set

comment:10 by Johannes Maron, 4 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:11 by Carlton Gibson, 4 years ago

I'm looking at the PR now, but I'll put this initial comment here, so it doesn't get lost:

Change the position of hidden inputs. Currently, they are injected at a certain index. I'd like to simply amend them. It would greatly simplify the templates. Since they are hidden, it shouldn't change browser rendering.

From experience with Crispy Forms, I'm doubtful that we can just move hidden fields to the end. Rendering, yes, would be unaffected. But the resultant data-dict would not be. With multi-value fields, as first thought, order does matter. (I can't count the number of times we've seen issues resolving around exact rendered field order on Crispy Forms...)

Open to be shown wrong here but... 😬

Last edited 4 years ago by Carlton Gibson (previous) (diff)

comment:12 by Carlton Gibson, 4 years ago

OK, scrub that last comment: existing rendering is already placing hidden fields last, so data-dict is unaffected too.

comment:13 by Johannes Maron, 4 years ago

Yes, I can confirm this is the case. I actually prefer the new HTML output, since we don't just amend hidden fields within the parent of another field.

So all this needs is a thorough review ;) Can't review it myself, but I am trading :P

comment:14 by Carlton Gibson, 4 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:15 by Johannes Maron, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:16 by Carlton Gibson, 3 years ago

Needs documentation: unset

I'm going to uncheck the Needs Documentation to put this back in the review queue.

comment:17 by Carlton Gibson, 3 years ago

Patch needs improvement: set

comment:18 by Johannes Maron, 3 years ago

Patch needs improvement: unset

comment:19 by Johannes Maron, 3 years ago

Owner: changed from nobody to Johannes Maron

comment:20 by Shai Berger, 3 years ago

Patch needs improvement: set

comment:21 by David Smith, 3 years ago

Owner: changed from Johannes Maron to David Smith
Patch needs improvement: unset

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 91e8b95d:

Refs #31026 -- Moved Template tests to separate class.

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 4ca508a6:

Refs #31026 -- Added extra form render tests.

comment:24 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 456466d:

Fixed #31026 -- Switched form rendering to template engine.

Thanks Carlton Gibson, Keryn Knight, Mariusz Felisiak, and Nick Pope
for reviews.

Co-authored-by: Johannes Hoppe <info@…>

comment:26 by GitHub <noreply@…>, 3 years ago

In 881a479:

Refs #31026 -- Fixed forms_tests if Jinja2 is not installed.

comment:27 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In bc1fa8eb:

[4.0.x] Refs #31026 -- Fixed forms_tests if Jinja2 is not installed.

Backport of 881a4799114fccefbc0f56c6524110ede2682e16 from main

comment:28 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 9be36f8:

Refs #31026 -- Improved BoundField.label_tag() docs.

comment:29 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In fc325a19:

[4.0.x] Refs #31026 -- Improved BoundField.label_tag() docs.

Backport of 9be36f8044c3bdfe5d1819d4b3b62bee64a039e3 from main

comment:30 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 78f062f6:

Refs #31026 -- Updated TemplatesSetting docs to refer to forms.

comment:31 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In a0e01b00:

[4.0.x] Refs #31026 -- Updated TemplatesSetting docs to refer to forms.

Backport of 78f062f63e7dea09c219fd1310d43950817f4c78 from main

comment:32 by GitHub <noreply@…>, 2 years ago

In 03a64881:

Refs #31026 -- Changed @jinja2_tests imports to be relative.

comment:33 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 3f5d43ce:

[4.0.x] Refs #31026 -- Changed @jinja2_tests imports to be relative.

Backport of 03a648811615cb623affc2d79dccd4b05919319e from main

comment:34 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

In 182d25e:

Refs #31026 -- Removed BaseForm._html_output() per deprecation timeline.

comment:35 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

In 31878b4d:

Refs #31026 -- Removed ability to return string when rendering ErrorDict/ErrorList.

Per deprecation timeline.

comment:36 by GitHub <noreply@…>, 5 months ago

In f1697ec:

Refs #31026 -- Simplified BaseForm.get_context().

bf.errors returns an ErrorList. Access this directly and avoid creating
a new instance in BaseForm.get_context()

Calling str() on the ErrorList can also be deferred to when the
variable used in the template.

Note: See TracTickets for help on using tickets.
Back to Top