Opened 2 months ago

Closed 2 weeks ago

#28585 closed Cleanup/optimization (fixed)

Set `has_file_field` context attribute for admin change forms conditionally on Form.is_multipart()

Reported by: Mark Rogaski Owned by: nobody
Component: contrib.admin Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In django.contrib.admin.options.ModelAdmin:

    def render_change_form(self, request, context, add=False, change=False, form_url='', obj=None):
        ...
        context.update({
            ...
            'has_file_field': True,  # FIXME - this should check if form or formsets have a FileField,
            ...

This attribute is used to determine whether a multipart/form-data is used for the form content type instead of application/x-www-form-urlencoded.

I'd like to implement this behavior as it appears to have been intended (and I have a situation where multipart forms should only be used if necessary for file uploads). However, it's likely that some applications might be adversely impacted if the behavior changes. Perhaps adding an AVOID_MULTIPART_ADMIN_FORMS setting or an avoid_multipart ModelAdmin attribute that defaults to the current behavior is appropriate (better suggestions for the naming are welcome).

In any case, I'd like to be able to have non-multipart forms returned when multipart isn't needed without having to subclass ModelAdmin.

I am also happy to submit a PR for this based upon the preferred approach to backward compatibility.

Change History (6)

comment:1 Changed 8 weeks ago by Tim Graham

Summary: Set `has_file_field` context attribute for admin change forms conditionally on form `is_multipart` methodSet `has_file_field` context attribute for admin change forms conditionally on Form.is_multipart()
Triage Stage: UnreviewedAccepted
Type: New featureCleanup/optimization

I think the check can reasonably use Form.is_multipart() and expected users to provide a custom form with that method overridden if needed. Do you have a case in mind where this isn't sufficient?

comment:2 Changed 8 weeks ago by Mark Rogaski

I used Form.is_multipart() on an instantiated form in a mixin and that worked fine, but it doesn't appear to recognize file fields in any inlines.

comment:3 Changed 8 weeks ago by Tim Graham

Perhaps my comment implied that checking 'has_file_field': form.is_multipart() is sufficient. I think the check for has_file_field will also have to check is_multipart() for the forms on the inline formset.

comment:4 Changed 8 weeks ago by Mark Rogaski

I'll put together a patch. I'd looked at it earlier and got stuck in the weeds a bit with the form introspection, but I'll look at it again over the weekend.

comment:5 Changed 4 weeks ago by Claude Paroz

Has patch: set

comment:6 Changed 2 weeks ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 0cf0076:

Fixed #28585 -- Calculated admin's change form multipart context variable from forms and formsets

Thanks Tim Graham for the review.

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