Opened 9 months ago

Last modified 5 months ago

#31026 assigned Cleanup/optimization

Switch form rendering to template engine

Reported by: Johannes Hoppe Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
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 (14)

comment:1 Changed 9 months ago by Johannes Hoppe

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 Changed 9 months ago by Simon Charette

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.

Last edited 9 months ago by Simon Charette (previous) (diff)

comment:3 Changed 9 months ago by Johannes Hoppe

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 Changed 9 months ago by Johannes Hoppe

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 Changed 9 months ago by Johannes Hoppe

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 Changed 9 months ago by Johannes Hoppe

Patch needs improvement: set

comment:7 Changed 8 months ago by Johannes Hoppe

Patch needs improvement: unset

comment:8 Changed 8 months ago by Johannes Hoppe

Needs documentation: set

comment:9 Changed 8 months ago by Johannes Hoppe

Patch needs improvement: set

comment:10 Changed 8 months ago by Johannes Hoppe

Needs documentation: unset
Patch needs improvement: unset

comment:11 Changed 8 months ago by Carlton Gibson

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 8 months ago by Carlton Gibson (previous) (diff)

comment:12 Changed 8 months ago by Carlton Gibson

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

comment:13 Changed 8 months ago by Johannes Hoppe

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 Changed 5 months ago by Carlton Gibson

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top