Problem/Motivation
A site builder is able to allow the user to enter a description for the file field upload. However, in some circumstances, the description may be mandatory. Right now there's no way for a site builder to make the file field upload description mandatory.
Proposed resolution
Add a new setting for file items to require the file description.
Remaining tasks
Updated 5/23/23
See #85
- Framework manager review
- Add JavaScript test coverage for #states that support the progressive disclosure
- Manual testing
- Update the upgrade path tests. Is this addressed by f0a74cb? Or is there more work to do here?
- Add REST/JSON:API test coverage
- Question from manual testing in #100: If the description field is required for a media item, should it be available on file upload when you have a Media field?
User interface changes
A new file field setting that allows to enforce the description.
Site builder perspective
End-user perspective
API changes
None.
Data model changes
New, description_field_required
, boolean key in the mapping of code>field.field_settings.file schema.
Release notes snippet
As a site builder, on a file field settings form, after checking Enable Description field, check the Require the Description field setting in order to make the file upload required.
Original report
If I add a file field and check Description, I don't get the option to make Description a required field. This results in the public seeing often-unfriendly file names instead of a meaningful description.
Steps: As admin:
1. Edit basic page
2. Add new file field
Actual result: Description is not prechecked
Expected result: Optionally, Description is pre-checked as the public-friendliest solution
3. Check Description
Actual result: No option to require description appears
Expected result: Check box to require description appears. Optionally, it is pre-checked as the public-friendliest solution.
As editor:
1. Add new basic page
2. Attach file
3. Don't fill in Description field
4. Save and publish
Actual result: No error
Expected result: Error
Comment | File | Size | Author |
---|---|---|---|
#82 | after_patch.png | 30.68 KB | marcusvsouza |
Issue fork drupal-2320877
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #4
pfrenssenThis 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.
Comment #5
pfrenssenHere is a proposed implementation.
Comment #7
pfrenssenAnd I though I finally understood schema definitions, nope.
Comment #9
pfrenssenMore test fixes.
Comment #11
pfrenssenFixed a coding standards violation, and hopefully also the installer test, I didn't try to run this locally.
Comment #12
pfrenssenSmall 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.Comment #14
pfrenssenFixed 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:
Comment #15
pfrenssenJust discovered that this now prevents the removal of a file, or replacing a file with another with the validation error "Description field is required".
Comment #16
pfrenssenSkipped the validation when uploading or removing files. I first tried to do this via
#limit_validation_errors
inFileWidget::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.
Comment #18
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedPatch 2320877-16.patch failed to apply. Can you check and update with the new patch
Comment #19
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedComment #20
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedre-rolled the patch to 8.5.x
Comment #22
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedPatch 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.
Comment #23
pfrenssenThis would break backwards compatibility with the current implementation.
Comment #26
claudiu.cristeaStraight reroll of #16.
Comment #28
claudiu.cristeaFixed the tests. Also reverted the change from #7 because
description_field_required
is only part of the file schema, together withdescription_field
.Comment #31
claudiu.cristeaReroll for 8.8.x
Comment #32
claudiu.cristeaAdding a new config key needs an update path, right?
According to
field.field_settings.file
schema this should be a boolean, not integer.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.All should
FALSE
. See their default settings.Let's modernize: s/get_class($this)/static::class
!array_diff() ?
Try to compact in a single line.
Comment #33
claudiu.cristeaFixes #32.
Comment #35
claudiu.cristeaHm, as
ImageItem
is extendingFileItem
, we still have to keep the check on settings existence as these default settings are removed inImageItem::defaultFieldSettings().
. But this time we're setting the correct type, which is boolean. Also, we should remove the new setting form element fromImageItem
.Comment #36
claudiu.cristeaComment #37
pfrenssenChanges look good to me but I cannot RTBC this since I made the initial implementation and this was not RTBC before.
Comment #40
MrPaulDriver CreditAttribution: MrPaulDriver commentedThis 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 :-)
Comment #41
claudiu.cristeaYou cannot RTBC a patch that is not passing the test. Most probably the patch needs a reroll against 9.2.x
Comment #42
MrPaulDriver CreditAttribution: MrPaulDriver commentedThank you for correcting me.
Is this something you could look at @claudiu.cristea ?
Comment #43
anushrikumari CreditAttribution: anushrikumari at OpenSense Labs commentedComment #44
anushrikumari CreditAttribution: anushrikumari at OpenSense Labs commentedRerolled patch #35 for 9.2.x
Comment #45
claudiu.cristeaThis was once RTBCed, in #40. I’m only setting back after reroll.
Comment #46
MrPaulDriver CreditAttribution: MrPaulDriver commentedI am finding that patch #44 does not apply with composer.
Comment #47
claudiu.cristeaProbably you’re not on 9.2.x. If applies in #45, then it works also with Composer.
Comment #48
alexpottWe need a change record for this change.
This patch has some coding standards issues.
Comment #49
claudiu.cristeaAdded change record. Fixed minor code standards issues.
Comment #50
alexpottComment #51
claudiu.cristeaReverting a too far change.
Comment #52
alexpottAdded the issue summary template. It could do with being properly completed.
Comment #53
claudiu.cristeaUpdated the IS according to template.
Comment #54
xjmThanks for your work on this issue! I've a couple questions about it:
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:
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.Comment #55
xjmUnrelatedly, this is sort of false:
Additions to the config schemata are data model additions and should probably be documented there in the IS.
Comment #56
alexpottI 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.
Comment #57
alexpottIf #56 is done then this code should be obsolete.
This changes of value are a bit odd - why is this being done here apart from for "consistency"?
Comment #58
MrPaulDriver CreditAttribution: MrPaulDriver commentedHas anyone had chance to do further work on this?
Comment #60
claudiu.cristea@xjm
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.@alexpott
Ready for another review.
Comment #61
claudiu.cristeaTo whom it may concern: here's an 8.9.x unofficial version. This patch is not for review.
Comment #62
pfrenssenI 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.
Comment #63
benjifisherUsability review
We discussed this issue at #3200598: Drupal Usability Meeting 2021-02-26.
From #54:
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.
Comment #64
MrPaulDriver CreditAttribution: MrPaulDriver commentedGood to see this was endorsed by the UX group.
Is there anything to prevent this finding it's way in the core?
Comment #65
claudiu.cristeaFixed 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.
Comment #66
MrPaulDriver CreditAttribution: MrPaulDriver commentedSorry. Not up to speed with the new MR workflow.
Comment #67
Madhu kumar CreditAttribution: Madhu kumar as a volunteer and at Zyxware Technologies commentedCan't apply patch #44 on 9.2.x , added screenshot for the reference.
Comment #68
claudiu.cristea@MrPaulDriver, that's very simple:
composer.json
which is the MR URL followed by.diff
extension. In this case you should append.diff
tohttps://git.drupalcode.org/project/drupal/-/merge_requests/282
, so your patch will be https://git.drupalcode.org/project/drupal/-/merge_requests/282.diffComment #69
claudiu.cristea@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.
Comment #71
scoisne CreditAttribution: scoisne commentedHello, 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.
I'm trying on a fresh D9 install (with only cweagans/composer-patches for applying patches)
I'm missing something ?
Comment #72
GuillaumeG CreditAttribution: GuillaumeG as a volunteer commentedMerging 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.
Comment #73
anweshasinha CreditAttribution: anweshasinha at Valuebound commentedI am working in this
Comment #74
anweshasinha CreditAttribution: anweshasinha at Valuebound commentedHi,
I have worked on this. Please review my work.
Comment #75
anweshasinha CreditAttribution: anweshasinha at Valuebound commentedComment #76
anweshasinha CreditAttribution: anweshasinha at Valuebound commentedMade certain changes in the patch. The updated one is given below.
Comment #77
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed custom command failure
Comment #78
AaronMcHaleSince 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.
Comment #79
paulocsThe MR should be actually merged into 9.3.x branch.
Comment #81
paulocsI 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.
Comment #82
marcusvsouza CreditAttribution: marcusvsouza at CI&T commentedThe MR in comment #80 works as expected, implemented in all types of media content with no code standards problems.
Image attached for reference.
Comment #83
marcusvsouza CreditAttribution: marcusvsouza at CI&T commentedChanging to RTBC!
Comment #85
xjmRemoving 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.
#states
use present that presumably supports the progressive disclosure, but not JavaScript test coverage for the behavior. We should add that.Thanks everyone!
Comment #86
xjmAlso adding credit for helpful reviews. Thanks, reviewers!
Comment #90
megadesk3000 CreditAttribution: megadesk3000 at Unic commentedHello 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.
Comment #93
szato CreditAttribution: szato at Brainsum commentedUsing MR#3117 with 9.5.7, works. Thank you.
Comment #94
florianmuellerCHAs this affects core and I have the need to apply multiple patches, I created a patch for 10.x to apply.
Comment #97
capysara CreditAttribution: capysara at Bounteous commentedRe-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.
Comment #98
capysara CreditAttribution: capysara at Bounteous commentedComment #100
capysara CreditAttribution: capysara at Bounteous commentedManual testing (WIP)
I used the Live preview via Tugboat (TIL there's a Live preview via Tugboat!)
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.
Comment #101
capysara CreditAttribution: capysara at Bounteous commentedComment #104
malcomio CreditAttribution: malcomio at Capgemini commentedWith 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
Comment #105
Vanitha Sophia CreditAttribution: Vanitha Sophia as a volunteer commentedTo address the Undefined array key "description_field" in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement for issue 104, we can use this patch
Comment #106
guardiola86 CreditAttribution: guardiola86 commentedTested and it's working fine
Comment #107
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented@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.
Comment #109
claudiu.cristeaComment #110
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThere is a failure in the MR pipeline, which does not looks like a random one, but related to the MR changes:
https://git.drupalcode.org/issue/drupal-2320877/-/jobs/507087