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.