The "Enable Description field" setting for a file field instance (as defined in Drupal\file\Plugin\Field\FieldType\FileItem::fieldSettingsForm()) cannot be set via the UI. If this option is checked (set TRUE) it will not be preserved.

To reproduce:

  1. Install site instance and enable "node", "field_ui" and "file"
  2. Create a new node content type and add a field of type "file" (e.g., field_file) to it.
  3. When configuring the file field's instance settings, check the "Enable Description field" option and save.

The expected behavior is that a "description" (sub)field will appear while uploading a file to the field_file field (when creating a new node), which can be populated with a string. This option does not appear. In addition, when navigating back to the field instance settings form for field_file, the "Enable Description field" is not checked. Re-checking it and re-saving always leads to this value being unset.

Solution:

It looks like the description_field form element that's defined in Drupal\file\Plugin\Field\FieldType\FileItem::fieldSettingsForm() contains a "#parents" value, which seems to have been carried over from this form element's definition in D7. I believe that this "#parents" value simply needs to be removed for the setting to property get saved.

Note that overall behavior of this description field seems to have test coverage (in \Drupal\file\Tests\FileFieldDisplayTest::testNodeDisplay()), but in that test the description_field value is set with entity_create(), as opposed to the field creation forms, so the behavior of the form itself is not tested.

CommentFileSizeAuthor
#24 file_field-enable_description-2350683-24-TEST_ONLY.patch2.28 KBrjacobs
#24 file_field-enable_description-2350683-24.patch3.04 KBrjacobs
#21 file_field-enable_description-2350683-21.patch3.07 KBrjacobs
#21 file_field-enable_description-2350683-21-TEST_ONLY.patch2.31 KBrjacobs
#17 file_field-enable_description-2350683-17-TEST_AND_PATCH.patch3.16 KBrjacobs
#17 file_field-enable_description-2350683-17-TEST_ONLY.patch2.4 KBrjacobs
#17 file_field-enable_description-2350683-17.patch774 bytesrjacobs
#6 file_field-enable_description-2350683-6-TEST_AND_PATCH.patch3.39 KBrjacobs
#6 file_field-enable_description-2350683-6-TEST_ONLY.patch2.64 KBrjacobs
#1 file_field_enable_description_field_not_saved_in_ui-2350683-1.patch774 bytesrjacobs

Comments

rjacobs’s picture

rjacobs commented
Status: Active » Needs review
StatusFileSize
new file_field_enable_description_field_not_saved_in_ui-2350683-1.patch774 bytes

Here's a simple patch to remove the "#parents" value as noted in solution part of the OP.

Should I also add a test for this? The problem is isolated to the form definition so I'm not sure if that justifies a dedicated test method. If as test is needed, would it be bundled for inclusion in the permanent test cases, or just used temporarily to help rectify this specific issue?

Status: Needs review » Needs work

The last submitted patch, 1: file_field_enable_description_field_not_saved_in_ui-2350683-1.patch, failed testing.

Status: Needs work » Needs review

rjacobs queued 1: file_field_enable_description_field_not_saved_in_ui-2350683-1.patch for re-testing.

rjacobs’s picture

rjacobs commented

#2364701: Description field for file field can't be enabled is marked as a dup. The same patch was proposed in that issue as well.

rjacobs queued 1: file_field_enable_description_field_not_saved_in_ui-2350683-1.patch for re-testing.

rjacobs’s picture

rjacobs commented
StatusFileSize
new file_field-enable_description-2350683-6-TEST_ONLY.patch2.64 KB
new file_field-enable_description-2350683-6-TEST_AND_PATCH.patch3.39 KB

Ok, I went ahead and whipped-up a test to demonstrate this programmatically. I'm attaching a test only version along with a version that includes the test and the patch. By no means do I think the test should be part of a commit (the patch-only file from #1 is still the candidate solution), this is just to hopefully streamline a review process so as to get this RTBC.

The last submitted patch, 6: file_field-enable_description-2350683-6-TEST_ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: file_field-enable_description-2350683-6-TEST_AND_PATCH.patch, failed testing.

Status: Needs work » Needs review

rjacobs queued 6: file_field-enable_description-2350683-6-TEST_AND_PATCH.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: file_field-enable_description-2350683-6-TEST_AND_PATCH.patch, failed testing.

Status: Needs work » Needs review

rjacobs queued 6: file_field-enable_description-2350683-6-TEST_AND_PATCH.patch for re-testing.

rjacobs queued 6: file_field-enable_description-2350683-6-TEST_ONLY.patch for re-testing.

The last submitted patch, 6: file_field-enable_description-2350683-6-TEST_ONLY.patch, failed testing.

rjacobs’s picture

rjacobs commented

Just my luck, the testbot was malfunctioning when I submitted my tests. Anyway, it seems to be alright now and after a test re-submission the results are as expected.

For ongoing reference regarding the core issues queues I would still be interested to know if these kinds of simple UI issues actually warrant a test. I'd like to make things as easy to review as possible, but it seems to me that a 1-liner FAPI oversight like this may not actually justify a supporting test?

benjy’s picture

benjy commented 
  1. +++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
@@ -93,4 +93,55 @@ function testNodeDisplay() {
+  ¶
...
+    // Use the UI to add a new content type that also contains a field field.
...
+  ¶

    Type, "field field"

  2. +++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
@@ -93,4 +93,55 @@ function testNodeDisplay() {
+    // Get ID of the newly created node from the current URL.
+    $matches = array();
+    preg_match('/node\/([0-9]+)/', $this->getUrl(), $matches);

    I think what you want here is $this->drupalGetNodeByTitle() 

  3. +++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
@@ -93,4 +93,55 @@ function testNodeDisplay() {
+  ¶
...
+  ¶

    Un-needed white space.

EDIT: Sorry, I should have answered your question, we do usually add tests for regressions such as this.

rjacobs’s picture

rjacobs commented

Sorry, I should have answered your question, we do usually add tests for regressions such as this.

Ok, I guess that makes sense. I'll clean-up the test a bit based on your comments. Thanks.

rjacobs’s picture

rjacobs commented
StatusFileSize
new file_field-enable_description-2350683-17.patch774 bytes
new file_field-enable_description-2350683-17-TEST_ONLY.patch2.4 KB
new file_field-enable_description-2350683-17-TEST_AND_PATCH.patch3.16 KB

Ok, here are some re-rolls that factor in comments from #15. Thanks for those notes.

I'm still of the mindset that committing the test could be unnecessary, but either way I wanted to get it updated accordingly.

The last submitted patch, 17: file_field-enable_description-2350683-17-TEST_ONLY.patch, failed testing.

arla’s picture

arla commented

Looks good. But I would guess that the cache-clears in the test are not needed.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
@@ -93,4 +93,49 @@ function testNodeDisplay() {
+    // Clear some caches for good measure.
+    $entity_manager = $this->container->get('entity.manager');
+    $entity_manager->getStorage('field_storage_config')->resetCache();
+    $entity_manager->getStorage('field_config')->resetCache();

Yes, it's not. Because there are no such caches.

Config entities are only cached as config objects and automatically invalidated already. config entities have no static or persistent cache unless they explicitly opt-in, and those two don't.

rjacobs’s picture

rjacobs commented
Status: Needs work » Needs review
StatusFileSize
new file_field-enable_description-2350683-21-TEST_ONLY.patch2.31 KB
new file_field-enable_description-2350683-21.patch3.07 KB

Right-o. Here's a version with the cache clears removed. I also needed to modify the name attribute used to target the description field in the test (field[settings][description_field] to settings[description_field]) as it looks like some form element nesting changed in core since the last patch.

The last submitted patch, 21: file_field-enable_description-2350683-21-TEST_ONLY.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
@@ -120,4 +120,45 @@ function testDefaultFileFieldDisplay() {
+    $inst_bundle = 'test';
+    $inst_field_type = 'file';
+    $inst_field_name = 'field_file';

Sorry, didn't see this before.

We usually don't shorten variable names, I'm also not sure why we even need the inst prefix?

Why not just $node_type = 'test' and $field_type and $field_name?

rjacobs’s picture

rjacobs commented
Status: Needs work » Needs review
StatusFileSize
new file_field-enable_description-2350683-24.patch3.04 KB
new file_field-enable_description-2350683-24-TEST_ONLY.patch2.28 KB

We usually don't shorten variable names, I'm also not sure why we even need the inst prefix?

Your absolutely right. The "inst" prefix was just something I was in the habit of using on contrib tests. Anyway, when I whipped-up this test I really didn't think it would be considered for inclusion along with the fix (I was just trying to validate the issue programmatically). It didn't become clear until #15 that these kinds of very simple UX regressions/oversights would also be candidates for a dedicated core test method. Long story short, my initial test may have been a bit sloppy.

I've made some adjustments to (hopefully) get things more in-line with other core test naming conventions. I'm now using the same names found elsewhere in FileFieldDisplayTest.php.

Status: Needs review » Needs work

The last submitted patch, 24: file_field-enable_description-2350683-24-TEST_ONLY.patch, failed testing.

arla’s picture

arla commented
Status: Needs work » Needs review

A trick I recently learned: Upload test-only patch before real patch to avoid this kind of "Needs work" status change.

rjacobs’s picture

rjacobs commented

@arla, thanks for that tip (and for resetting the correct status). I'm actually surprised that quirk of d.o. has not tripped me up more in the past.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good now I think.

This is a plain bug fix that has no impact on an other code so I don't think we need a beta evaluation for this?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed e1c2401 and pushed to 8.0.x. Thanks!

  • alexpott committed e1c2401 on 8.0.x 
    Issue #2350683 by rjacobs, Berdir: File field "Enable Description field...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.