Add a setting to make description a required field for file items

This is a very good suggestion. Thanks also for the excellently written feature request.

I would propose to default to _not_ requiring the description field. That will make it fully backwards compatible with the current implementation and we won't need to create an update path.

Assigning to me to make a first draft of this feature request.

Here is a proposed implementation.

And I though I finally understood schema definitions, nope.

More test fixes.

Fixed a coding standards violation, and hopefully also the installer test, I didn't try to run this locally.

Small improvement: the image field extends the file field but has its own "title" field that replaces the description. The description_field_required configuration parameter is not used by the image field so we should unset it.

Fixed the new installer error as well as a notice that occurs when submitting a form containing a required description on a non-required file field and not populating the file field:

Notice: Undefined index: description in Drupal\file\Plugin\Field\FieldWidget\FileWidget::validateRequiredDescription()

Just discovered that this now prevents the removal of a file, or replacing a file with another with the validation error "Description field is required".

Skipped the validation when uploading or removing files. I first tried to do this via #limit_validation_errors in FileWidget::process() but this wasn't really feasible, since we would need to limit validation errors on the whole file field _except_ the description child element. There is no #skip_validation_errors to selectively skip validation for a particular element.

Also fixed the other install error.

Patch 2320877-16.patch failed to apply. Can you check and update with the new patch

re-rolled the patch to 8.5.x

Patch form #20 applied successfully. But,
- The Description is not pre-checked as the public-friendliest solution
- When the "Enable Description field" is checked , "Require the Description field" option displays which is working as expected.

The Description is not pre-checked as the public-friendliest solution

This would break backwards compatibility with the current implementation.

Straight reroll of #16.

Fixed the tests. Also reverted the change from #7 because description_field_required is only part of the file schema, together with description_field.

Reroll for 8.8.x

1. +++ b/core/modules/file/config/schema/file.schema.yml
   @@ -69,6 +69,9 @@ field.field_settings.file:
   +    description_field_required:
   

   Adding a new config key needs an update path, right?

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -51,6 +51,7 @@ public static function defaultFieldSettings() {
          'description_field' => 0,
   

   According to field.field_settings.file schema this should be a boolean, not integer.

3. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -193,6 +194,19 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +      '#default_value' => isset($settings['description_field_required']) ? $settings['description_field_required'] : '',
   

   This is wrong. It should default to FALSE, not "", as is a boolean. But we don't need the ternary operator because we're covered with sane defaults.

4. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -214,6 +214,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
          'display_default' => NULL,
          'display_field' => NULL,
          'description_field' => NULL,
   +      'description_field_required' => NULL,
   

   All should FALSE. See their default settings.

5. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -267,6 +269,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      $element['#element_validate'][] = [get_class($this), 'validateRequiredDescription'];
   

   Let's modernize: s/get_class($this)/static::class

6. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -333,6 +338,33 @@ public static function value($element, $input, FormStateInterface $form_state) {
   +    if (array_diff($parent, array_intersect($parent, $trigger_parents)) === []) {
   

   !array_diff() ?

7. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -333,6 +338,33 @@ public static function value($element, $input, FormStateInterface $form_state) {
   +    // Throw a validation error if a file is present but the description is
   +    // empty.
   

   Try to compact in a single line.

Fixes #32.

Hm, as ImageItem is extending FileItem, we still have to keep the check on settings existence as these default settings are removed in ImageItem::defaultFieldSettings().. But this time we're setting the correct type, which is boolean. Also, we should remove the new setting form element from ImageItem.

Changes look good to me but I cannot RTBC this since I made the initial implementation and this was not RTBC before.

This patch solves a common problem for document heavy Drupal sites.

Tested against core versions 8.9.9 and 9.0.9. Patch applies without any problems.

As well as generic files, this works with the media library and media library modal. It is also filterable in the media library if the field_media_file_description identifier is used.

This also solves problems that the media module creates, because the inconsistency of how the media name base field is generated when uploading files directly to the media library versus via the media library modal. This inconsistency also results "in the public (and editors) seeing often-unfriendly file names instead of a meaningful description".

Please commit. This should have been in core ten years ago :-)

You cannot RTBC a patch that is not passing the test. Most probably the patch needs a reroll against 9.2.x

Thank you for correcting me.

Is this something you could look at @claudiu.cristea ?

Rerolled patch #35 for 9.2.x

This was once RTBCed, in #40. I’m only setting back after reroll.

I am finding that patch #44 does not apply with composer.

Probably you’re not on 9.2.x. If applies in #45, then it works also with Composer.

We need a change record for this change.

This patch has some coding standards issues.

FILE: /Users/alex/dev/drupal/core/modules/file/file.post_update.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 18 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 425ms; Memory: 8MB


FILE: /Users/alex/dev/drupal/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
--------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------
 196 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------

Time: 3.76 secs; Memory: 10MB

-e PHPCS: core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php passed

FILE: /Users/alex/dev/drupal/core/modules/file/tests/src/Functional/FileFieldDisplayTest.php
--------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------
 187 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------

Time: 2.66 secs; Memory: 10MB


FILE: /Users/alex/dev/drupal/core/modules/file/tests/src/Functional/Update/FileDescriptionRequiredUpdateTest.php
----------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------
 44 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------------

Added change record. Fixed minor code standards issues.

Reverting a too far change.

Added the issue summary template. It could do with being properly completed.

Updated the IS according to template.

Thanks for your work on this issue! I've a couple questions about it:

1. Does this option need to be added in core? It is a fairly trivial form alter for contrib or custom code to support this option.
2. Even if the configuration setting is provided in core, do we need to expose it in the core UI? More form fields, descriptions, text, etc. is all detrimental to usability, so we need to weigh the usability improvement of site owners easily requiring readable filenames against the usability cost of adding the form field.

Tagging for usability review to (a) Help decide where the usability gain offsets the cost and (b) Review the UI change itself.

Also, one improvement that I think we don't even need a usability team review to make:

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
@@ -188,11 +189,24 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+      '#description' => $this->t('Whether or not the description field is required.'),

This description is redundant with the label and should be removed. Our guidance for form elements is that, where possible, descriptions should be omitted in favor of including all relevant info in the label. The label by itself is already sufficient here.

You can get additional feedback for usability by joining the #ux channel in Drupal Slack, and in particular from the weekly UX meeting Fridays at 1500 UTC.

Unrelatedly, this is sort of false:

Data model changes
None.

Additions to the config schemata are data model additions and should probably be documented there in the IS.

I think in order to do this so that the required-ness also works for JsonAPI and on entity validation we need to implement a \Drupal\file\Plugin\Field\FieldType\FileItem::getConstraints() and add a constraint on the field to check that the description is not empty.

1. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -267,6 +269,10 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if (!empty($element['#description_field_required'])) {
   +      $element['#element_validate'][] = [static::class, 'validateRequiredDescription'];
   +    }
   
   @@ -332,6 +338,32 @@ public static function value($element, $input, FormStateInterface $form_state) {
   +  /**
   +   * #element_validate callback to check if a required description is filled in.
   +   *
   +   * @param array $element
   +   *   An associative array containing the properties and children of the
   +   *   generic form element.
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *   The current state of the form.
   +   * @param array $complete_form
   +   *   The complete form structure.
   +   */
   +  public static function validateRequiredDescription(array &$element, FormStateInterface $form_state, array &$complete_form) {
   +    // Skip validation if a file is being uploaded or deleted.
   +    $parent = array_slice($element['#parents'], 0, -1);
   +    $trigger_parents = $form_state->getTriggeringElement()['#parents'];
   +    if (!array_diff($parent, array_intersect($parent, $trigger_parents))) {
   +      if (in_array(array_pop($trigger_parents), ['upload_button', 'remove_button'], TRUE)) {
   +        return;
   +      }
   +    }
   +    // Throw a validation error when the file is present but has no description.
   +    if (!empty($element['fids']['#value']) && empty($element['description']['#value'])) {
   +      $form_state->setError($element, t('@name field is required.', ['@name' => $element['description']['#title']]));
   +    }
   +  }
   

   If #56 is done then this code should be obsolete.

2. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -210,9 +210,10 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -      'display_default' => NULL,
   -      'display_field' => NULL,
   -      'description_field' => NULL,
   +      'display_default' => FALSE,
   +      'display_field' => FALSE,
   +      'description_field' => FALSE,
   

   This changes of value are a bit odd - why is this being done here apart from for "consistency"?

Has anyone had chance to do further work on this?

@xjm

• #54.1, 2: This is more or less symmetrical to title_field_required from image item. And if that exists in core and is exposed in UI, I see no reason why this one shouldn't be.
• #54: True. Removed the useless description.
• #55: Fixed IS.

@alexpott

• #56, #57.1: Yeah, very elegant. Done. I've also added a JSON:API test.
• #57.2: Yes, it's consistency and also the settings semantics whispers me that those are booleans. I agree that are out of scope, it was just an easy win with no BC implications.

Ready for another review.

To whom it may concern: here's an 8.9.x unofficial version. This patch is not for review.

I found a few nitpicks but apart from these this is looking good. I think this probably fine from a usability perspective since this functionality is similar to what we have in image fields. Anyway let's keep the usability review tag so this can be discussed at the UX meeting.

Usability review

We discussed this issue at #3200598: Drupal Usability Meeting 2021-02-26.

From #54:

1. Does this option need to be added in core? It is a fairly trivial form alter for contrib or custom code to support this option.
2. Even if the configuration setting is provided in core, do we need to expose it in the core UI? More form fields, descriptions, text, etc. is all detrimental to usability, so we need to weigh the usability improvement of site owners easily requiring readable filenames against the usability cost of adding the form field.

Without doing any surveys nor user testing we are just guessing, but the consensus is that this feature will be popular enough that it is worth adding to core. Part of that opinion is that a hypothetical contrib module that just provides this change would be hard to discover. I know that @phenaproxima has suggested Media Library Extras as a home for such little tweaks, but since this issue is concerned with file fields, which may or may not be attached to media, that does not seem appropriate.

We did not discuss the second question at the meeting. My personal opinion is that the "Require the Description field" checkbox should be exposed only when "Enable Description field" is checked (progressive disclosure). I think the current patch already works this way. Given that, there is no extra clutter by default, and the additional field is exposed at exactly the right time.

Good to see this was endorsed by the UX group.

Is there anything to prevent this finding it's way in the core?

Fixed MR remarks from @pfrenssen.

@MrPaulDriver, the best way to get this in core, is to help test the latest MR version and do a code review. Then, if satisfied, to mark as RTBC.

Sorry. Not up to speed with the new MR workflow.

Can't apply patch #44 on 9.2.x , added screenshot for the reference.

@MrPaulDriver, that's very simple:

• If you're testing on a vanilla Drupal, just clone this branch https://git.drupalcode.org/issue/drupal-2320877/-/tree/2320877-descripti...
• If you test in your project you have to apply the patch in composer.json which is the MR URL followed by .diff extension. In this case you should append .diff to https://git.drupalcode.org/project/drupal/-/merge_requests/282, so your patch will be https://git.drupalcode.org/project/drupal/-/merge_requests/282.diff

@Madhu kumar, #44 is outdated, the 9.2 code is in MR. Please don't post anymore the screenshots, they are useless and only clutters the issue. We can determine if a patch applies by running the tests.

Hello, i may be dumb but the MR !282 don't seems to be a valid patch for the 9.2.0 even if the test are correct.
I'm trying to apply with composer with no luck so far.

        "patches": {
            "drupal/core": {
                "2320877 - Description required": "https://git.drupalcode.org/project/drupal/-/merge_requests/282.diff",
            },

I'm trying on a fresh D9 install (with only cweagans/composer-patches for applying patches)

I'm missing something ?

Merging latest 9.2.x branch from drupal repo and fixing conflits.

Ready to review / test.

GuillaumeG made their first commit to this issue’s fork.

I am working in this

Hi,
I have worked on this. Please review my work.

Made certain changes in the patch. The updated one is given below.

Fixed custom command failure

Since a merge request has been opened for this issue and has commits, please use that for adding changes/fixes instead of uploading patch files, otherwise we'll just end up with multiple branching versions of this and so wasted effort, thank you.

The MR should be actually merged into 9.3.x branch.

I was not able to close the MR 282. I created a new branch and opened a new MR into the 9.3.x Drupal branch.

The MR in comment #80 works as expected, implemented in all types of media content with no code standards problems.
Image attached for reference.

Changing to RTBC!

Removing credit for unnecessary patches and screenshots.

Sorry that this issue has sat for so long without feedback. Given that I missed #56 I would prefer a framework manager review the issue again prior to commit.

#63 seems reasonable; thanks @benjifisher and UX team.

1. There is #states use present that presumably supports the progressive disclosure, but not JavaScript test coverage for the behavior. We should add that.
2. It'd be good to also have clear confirmation from manual testing that the progressive disclosure works as expected when checking and un-checking the box.
3. The MR diff does not apply to 10.0.x. (It does apply to 9.4.x, so a new MR could be created for 9.4.x easily from the current diff. We need both here; 9.3.x is bugfix-only now.)
4. We also need the upgrade path tests updated to use the 9.3 fixtures (rather than the 8.8 ones).
5. Edit: REST/JSON:API test coverage would also be good given #56.

Thanks everyone!

Also adding credit for helpful reviews. Thanks, reviewers!

Hello together
Thanks for your work so far.
MR !975 was not applying against 9.4.3, so i rebased the changes on 9.4.x.

Using MR#3117 with 9.5.7, works. Thank you.

As this affects core and I have the need to apply multiple patches, I created a patch for 10.x to apply.

Re-rolled for 10.1.x. I based this MR on MR#3117 because the patch in the previous comment introduced some changes from the last version, most notably removing the constraints that were added in a previous commit. Maybe the patch was based on an earlier version?

I did not make any changes to the re-roll, so the updates noted in #85 still need to be implemented. I'm working on manually testing it.

Manual testing (WIP)

I used the Live preview via Tugboat (TIL there's a Live preview via Tugboat!)

1. Create a file field (field_myfile1) in the Basic page content type admin/structure/types/manage/page/fields/add-field
2. Check "Enable Description field"
3. "Require the Description field" displays and is not enabled
4. Uncheck "Enable..."
5. "Require..." field is not displayed & Save
6. Create a file field (field_myfile2)
7. Enable Description but don't Require & Save
8. Create a file field (field_myfile3)
9. Enable Description and Require & Save
10. Add Basic page node & Add files to fields
11. Save without adding a description to the required field_myfile3
12. Unable to save (fails both client- and server-side validation). Expected.
13. Add a description to myfile3 and save
14. Not good* Can't save because I didn't add a description to myfile1. When I added the field, I Enabled description, enabled Require, and then Disabled description, but didn't disable Require. Now it's expecting a value, but the Description field doesn't display.
    Same behavior with other entities tested: Block (Basic), User, and Media (types Document and Video).
    UPDATE: pushed a new commit to disable the description_field_required if description_field is not enabled, which fixes it.
    

1. Create new media type Newmedia3, Media source: File
2. Edit the File settings - Enable/Require description
3. Add media item of type Newmedia3 /media/add/newmedia3
4. Description is required
5. Edit basic page content type and add Media field (field_newmedia3), Media type: Newmedia3
6. There's nowhere to add a description. But that's how it's always been, right?
7. Add node of type Basic page, Add media to newmedia3 field. Choose file, there's only a Name field, no description field.
8. Should there be a description field on file upload (for Media fields when uploading through the media library) since the description is required? Is that out of scope for this ticket? Note: When you edit the media item itself, the Description field displays and is required.

With the patch from #94 applied, we have observed PHP warnings, similar to #3380377: Undefined array key "description_field_required" in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement:

Undefined array key "description_field" in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement

To address the Undefined array key "description_field" in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement for issue 104, we can use this patch

Tested and it's working fine

@guardiola86 Thanks for your review and the status change, but this issue should be a Needs Work at the moment.

Which patch/MR have you reviewed? If the one from #105, this patch does not apply. The bigger issue is, that the content of that patch is significantly different from the MR 3915 and MR 4255 (see for example the default label "Require Description field"). Therefore I am not sure what was the starting point for this patch. Also the patch is missing interdiff, so we are unable to see what changes were made.

It is good to use MRs in case these are already created and not to upload new patches.

@Vanitha Sophia Thanks for working on this issue, but at the start I suggest to read this contributor guide: https://www.drupal.org/community/contributor-guide/task/create-a-patch-f... before adding another patches here.

There is a failure in the MR pipeline, which does not looks like a random one, but related to the MR changes:

There was 1 error:
    
    1) Drupal\Tests\file\Functional\FileFieldDisplayTest::testDescToggle
    Behat\Mink\Exception\ElementNotFoundException: Button with
    id|name|label|value "Save and continue" not found.
    
    /builds/issue/drupal-2320877/core/tests/Drupal/Tests/WebAssert.php:144
    /builds/issue/drupal-2320877/core/tests/Drupal/Tests/UiHelperTrait.php:78
    /builds/issue/drupal-2320877/core/modules/file/tests/src/Functional/FileFieldDisplayTest.php:178
    /builds/issue/drupal-2320877/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

https://git.drupalcode.org/issue/drupal-2320877/-/jobs/507087