FilePathField should include blank option even when required=True

[ringemup@…]

Because there is no blank option in FilePathField form fields when required=True, it is not possible to save an admin form with blank inlines that contain FilePathFields. Since an empty-string option does not pass the required=True validator, the simplest fix is simply to include a (,'----------') option as the first choice for the field, just as with other ChoiceFields.

[Aymeric Augustin]

It took me a bit of time to figure out the problem; here's my analysis.

Django's forms have an (undocumented) empty_permitted attribute. When this attribute is set to True, validation is short-circuited (see lines 263-266 in django/forms/forms.py). Formsets need this internal API to display extra forms to add objects, but ignore them if they are submitted unchanged (empty). Specifically, the inlines feature of the admin uses this.

However, it isn't possible to submit a formset unchanged when it contains a FilePathField. This problem may affect other fields that can't be submitted with an empty value, given the UI (radio buttons, drop-down selects), when blank=False. The common point of these fields is that their value must be chosen from a finite set defined by their choices attribute.

In my opinion, the proper fix is to render fields as if blank was True when empty_permitted is True, probably by setting their include_blank attribute to True. Thus, the select widget for FilePathField will contain the blank choice, BLANK_CHOICE_DASH, as the first item, resolving the problem described originally.

Unfortunately, I failed to write a patch for this because I'm not sufficiently familiar with the forms implementation. After searching for all instances of include_blank and empty_permitted, I couldn't bridge the gap between them. This isn't an easy picking after all :)

[Mahmoud Nasser]

I already tested this ticket and the blank option is working fine.

I made the test with the following models:

class Parent(models.Model):
    name = models.CharField(max_length=100)

    def __str__(self):
        return self.name


class Child(models.Model):
    parent = models.ForeignKey(Parent, on_delete=models.CASCADE, related_name="children")
    # FilePathField with blank=True but required by form — we simulate it
    file_path = models.FilePathField(path="/tmp", match=".*\.txt$", recursive=False, blank=True)
    choices_field = models.CharField(max_length=100, choices=[("name", "name")])

    def __str__(self):
        return self.file_path or "No File"

And with the following admin:

# admin.py
from django.contrib import admin
from .models import Parent, Child

class ChildInline(admin.TabularInline):
    model = Child
    extra = 1  # Shows one empty inline form
    can_delete = True

@admin.register(Parent)
class ParentAdmin(admin.ModelAdmin):
    inlines = [ChildInline]

[Sarah Boyce]

If you believe this is fixed, this still might need a test to make sure this doesn't break in future.
We likely want to bisect to when this got fixed and to have others confirm that this is indeed fixed.
Once this has test coverage, we can close the ticket.

[Tim Graham]

Mahmoud, I don't think you reproduced this correctly. In your test model, you have FilePathField(..., blank=True) which suggests it would have required=False (rather than True) in forms.

[Mahmoud Nasser]

In the FilePathField we have the following line of code:

        if self.required:
            self.choices = []
        else:
            self.choices = [("", "---------")]

and when we pass the parmater from the model to the form we assign the required parameter based on blank that comming from the model as you could see here:

    def formfield(self, form_class=None, choices_form_class=None, **kwargs):
        """Return a django.forms.Field instance for this field."""
        defaults = {
            "required": not self.blank,
            "label": capfirst(self.verbose_name),
            "help_text": self.help_text,
        }
      ......

What I am planing to do is to cover this lines of code in the unittests as you could see in this commit: ​https://github.com/django/django/commit/a64a61bf4a which made the change in the FilePathField as I mentioned above , I will add tests for that change like this in test_filepathfield.py to confirm that it is working fine:

    def test_should_include_blank_option_when_required_is_true(self):
        f = FilePathField(path=self.path, match=r"^.*?\.py$", required=False)
        self.assertChoices(f, [('', '---------')] + self.expected_choices[:4])

if it is ok I will do a PR for above change.

[Harish Bonu]

PR submitted with regression tests to confirm the current behavior described in this ticket:

​https://github.com/django/django/pull/20812

The tests cover both admin inline submission and ModelFormSet behavior involving FilePathField(blank=False). CI is passing.

[blighj]

The behaviour seen on this ticket is caused because FilePathField doesn't have a blank 'Select an option' option inserted if it is required. Other choice fields will insert this option regardless of whether the field is marked required or not and the inline handling code knows to ignore a formset if all required fields are sent blank.
Because FilePathField doesn't have this blank option it will always be set to the first value, even in a new inline row. If there are no other required fields on that inline form, it will create a new row, which is what the regression test shows. If there are other required fields you won't be able to save the form, as the other fields validation will fail, which is what the reporter saw.

The fix is the same for both, FilePathField should always add the blank option, similar to other fields.

django/forms/fields.py

@@ -1200 +1200 @@
         self.set_choices()
 
     def set_choices(self):
-        if self.required:
-            self.choices = []
-        else:
-            self.choices = [("", get_blank_choice_label())]
+        self.choices = [("", get_blank_choice_label())]
 
         if self.match is not None:
             self.match_re = re.compile(self.match)

You can reproduce the broken behaviour with the following code

# models.py
import os

from django.db import models

# Any directory containing at least a couple of files works; the app dir will do.
FILES_DIR = os.path.dirname(__file__)


class BlankFalseParent(models.Model):
    name = models.CharField(max_length=100)


class BlankFalseChild(models.Model):
    parent = models.ForeignKey(BlankFalseParent, on_delete=models.CASCADE)
    path = models.FilePathField(path=FILES_DIR, match=r".*\.py$", blank=False)
    note = models.CharField(max_length=100)  # second required field


class BlankTrueParent(models.Model):
    name = models.CharField(max_length=100)


class BlankTrueChild(models.Model):
    parent = models.ForeignKey(BlankTrueParent, on_delete=models.CASCADE)
    path = models.FilePathField(path=FILES_DIR, match=r".*\.py$", blank=True)
    note = models.CharField(max_length=100)


class ChoicesParent(models.Model):
    name = models.CharField(max_length=100)


class ChoicesChild(models.Model):
    parent = models.ForeignKey(ChoicesParent, on_delete=models.CASCADE)
    choices_field = models.CharField(max_length=100, blank=False,  choices=[("name", "name")])
    note = models.CharField(max_length=100)


class StandaloneChoices(models.Model):
    choices_field = models.CharField(max_length=100, blank=False, choices=[("name", "name")])
    note = models.CharField(max_length=100)


class StandaloneRecord(models.Model):
    path = models.FilePathField(path=FILES_DIR, match=r".*\.py$", blank=False)
    note = models.CharField(max_length=100)

# admin.py
from django.contrib import admin

from .models import (
    BlankFalseChild, BlankFalseParent,
    BlankTrueChild, BlankTrueParent,
    ChoicesChild, ChoicesParent,
    StandaloneChoices, StandaloneRecord,
)


class BlankFalseChildInline(admin.StackedInline):
    model = BlankFalseChild
    extra = 1


class BlankTrueChildInline(admin.StackedInline):
    model = BlankTrueChild
    extra = 1


class ChoicesChildInline(admin.StackedInline):
    model = ChoicesChild
    extra = 1


@admin.register(BlankFalseParent)
class BlankFalseParentAdmin(admin.ModelAdmin):
    inlines = [BlankFalseChildInline]


@admin.register(BlankTrueParent)
class BlankTrueParentAdmin(admin.ModelAdmin):
    inlines = [BlankTrueChildInline]


@admin.register(ChoicesParent)
class ChoicesParentAdmin(admin.ModelAdmin):
    inlines = [ChoicesChildInline]


admin.site.register(StandaloneChoices)
admin.site.register(StandaloneRecord)