Problem/Motivation
When you create a file field, you can enable the description field for each file. This field description is supposed to override the file name on display.
It is working properly with the "Generic file" formatter but not with the "Table of files" formatter.
More than just fixing this bug (it was working in 7.x) we decided to add a setting on the concerned formatters to let the site builder choose if she wants the file name to be overriden by the description.
Proposed resolution
- pass the file description to the theme so it can be used instead of the file name
- add a setting to the "Generic file" formatter to use the description when available
- add a setting to the "Table of files" formatter to use the description when available
- add an upgrade path to disable the new settings for existing "Table of files" formatters (Backward Compatibility)
- increase test coverage to ensure these new settings are working
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | Done | |
Update the issue summary | Instructions | Done | |
Add automated tests | Instructions | Done | |
Manually test the patch | Novice | Instructions | Done |
Embed before and after screenshots in the issue summary | Novice | Instructions | Done |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | Done |
User interface changes
A new setting allows the site builder to define if she want the file name to be overriden by the description when it's available.
The file description is shown, when available and when the formatter allows it, instead of the file name when using the "Table of files" or "Generic files" formatter.
API changes
None.
Data model changes
A new setting on the file_table and file_default formatters.
Comment | File | Size | Author |
---|---|---|---|
#106 | file_description_formatter_setting-2677990-106.patch | 20.81 KB | DuaelFr |
#104 | interdiff.2677990.99.104.txt | 4.32 KB | DuaelFr |
#104 | file_description_formatter_setting-2677990-104.patch | 20.75 KB | DuaelFr |
#99 | interdiff.2677990.94.99.txt | 772 bytes | DuaelFr |
#99 | file_description_formatter_setting-2677990-99.patch | 20.78 KB | DuaelFr |
Comments
Comment #2
DuaelFrAdded steps to reproduce + patch fixing the issue.
I suppose we are going to need tests...
Comment #3
heykarthikwithu1. Installed drupal standard 8.0.x
2. Edit or create a content type
3. Add a file field with description enabled, with unlimited values (correct me if iam wrong)
4. Choose the "Table of files" formatter
5. Create a node, upload a file, add a description
5. Show the node
the file name is displayed and the description does not appear anywhere
After applying the patch, i was getting the same i.e file name itself was displayed.
Comment #4
DuaelFrDid you clear the cache after having the patch applied?
It works well on my Vanilla Drupal and on the project which needs this fix.
Comment #5
N.kishorekumar CreditAttribution: N.kishorekumar as a volunteer and at Valuebound commentedCreated file field, with unlimited values and selected formatter as TableFormatter
In node creation, uploaded the file and gave the description for the file
After Applying this patch, the file name is replaced with the description
Moving it to RTBC
Comment #6
catchPatch looks fine, but this needs test coverage.
Comment #7
heykarthikwithuworking on this
Comment #8
heykarthikwithuComment #10
gvsoThe test only evaluates files without description so far. I couldn't find a way of providing a description to the file in the test.
Changing status to "Needs review", so the bot can test it.
Comment #11
DuaelFrThanks you @gvso for starting this test.
Here is an upgrade that injects a description in the field then test that it is displayed with the default formatter and the "Table of files" formatter.
Comment #13
gvsoRBTC +1
Comment #14
alexpottAre we sure that we should change this behaviour on existing sites?
Comment #15
heykarthikwithu@alexpott yes, because this is same behaviour which is in 7.x :)
Comment #16
DuaelFrI really think this is an issue as you can ask for a description that's not shown nor even used anywhere with this formatter.
In my case, that's a client that asked me why it didn't work. I never expected it was on purpose.
Who could judge?
Edit: @heykarthikwithu good point!
Comment #17
alexpottIt's good to know this is how it was in 7.x.
So one thing that I think it confusing is the if you then go and disable the description test on the field the description is still used. This is the same for generic files. To me that is also unexpected. If the display used the description depending on whether that box was ticked that'd make more sense to me. Generic files will also keep using the description regardless of this setting.
Comment #18
DuaelFrShouldn't we open a follow-up issue for that particular thing?
It's going to need tests and also be delayed for a futur minor release.
Comment #19
heykarthikwithuactually I want to say this.
"Enable Description field" setting is at the field level (admin/structure/types/manage/article/fields/node.article.field_file_),
so description will be enable/disabled irrespective of fields formatter level setting (admin/structure/types/manage/article/display) this value may be 'Generic file', 'Table of formatter' or 'URL to file'.
So, IMO
Should we also have settings at field formatter level?? i.e for 'Table of formatter'
Comment #20
alexpott@DuaelFr the problem is we're changing existing behaviour which sites other than yours might now be relying on. And once we change this patch there's no way to get back to the way it used to behave without disabling the description and re-saving every node.
@heykarthikwithu I agree this should be a setting on the display - whether to use the description if it is available.
Comment #21
DuaelFrAccording to the last comments we have to:
- add a setting to the "Generic file" formatter to use the description when available (Enabled by default)
- add a setting to the "Table of files" formatter to use the description when available (Disabled by default)
- increase test coverage to ensure these new settings are working
Comment #22
alexpottWell it can default to enabled for new sites. But existing sites should not be changed.
Comment #23
DuaelFr@alexpott Sure but it would need an upgrade path and I don't understand how to write upgrade path tests ;)
Comment #24
alexpott@DuaelFr never too late to learn :) - any of the tests that extend UpdatePathTestBase is a good place to start. And whatever the defaults you'll probably need an upgrade path for both displays.
Comment #25
tstoecklerComment #26
hauruck CreditAttribution: hauruck at UEBERBIT GmbH commentedI have tested the patch from #11 on the 8.2.x branch and it didn't resolve the problem...
Comment #28
DuaelFrI'll spend a couple hours on this right now.
Comment #29
DuaelFrUpdated the title and the Issue Summary to reflect the new orientation of this issue.
Removed from the IS but kept in case it's needed:
Steps to reproduce the "Table of files" formatter issue
Expected result: the file name is replaced by the description
Current result: the file name is displayed and the description does not appear anywhere
Comment #30
DuaelFrComment #31
DuaelFrI added the tasks table in the issue summary.
Comment #34
DuaelFrFixed the tests and added upgrade tests.
*crossing fingers*
Comment #37
DuaelFrRequeueing the test as it looks like a random failure.
Comment #41
DuaelFrI finally got it!
Comment #44
DuaelFrComment #45
esolitosI read the patch in #41, applied the patch on a couple of sites and it all looks very good.
Comment #46
xjm#2654668: Show file description instead of filename if description has been provided was also in the RTBC queue; looks like a partial dupe. This issue offers a more complete solution, I think.
Also rearranging the files shown since an old patch is listed at the top.
Comment #48
DuaelFrReupload of patches from #41 to be tested against 8.3.x.
Comment #50
DuaelFrTestbot is happy. Let's go back to RTBC :)
Comment #51
alexpottThe config key and the label seem to be a mismatch to me.
show_description
implies that a description is going to additionally shown whereas the reality is that the description is used instead of the filename. Perhaps the setting should beuse_description_as_link_text
since this is what is happening.Comment #52
DuaelFr@alexpott Job done
Comment #57
DuaelFrTestbot is happy now. Let's go back to RTBC as the changes between #48 and #52 are minor.
Comment #58
Wim LeersNit: s/made/done/
Needs FQCN.
Nit: 80 cols violations.
Comment #59
alexpottAlso unnecessary capitalisation of Backward Compatibility.
Let's fix this and #58 and add a change record.
Comment #60
DuaelFrHere is the new patch that fixes #58 and #59.
Comment #61
DuaelFrAnd there is the change record https://www.drupal.org/node/2838648
I hope my english is not too bad :/
Comment #64
DuaelFrIt really looks like random failures o_O
Comment #65
DuaelFr\o/ It passed !
Go back to RTBC as it was only comment fixes.
Please review the attached change record too (I'm not good at documentation).
Comment #71
alexpottI've just spotted a problem we need to update the generic file formatters to but to have the setting set to TRUE. Because after this patch is applied if you just go to field settings and press save on a generic file field the configuration will change (to have settings.use_description_as_link_text set to TRUE). Therefore we need to handle this in the update as well.
Comment #72
DuaelFrHere we go!
Comment #75
DuaelFrRerolled against 8.4.x
Triggering test bot while doing some mentoring
Comment #77
DuaelFrWrong tag, sorry
Comment #78
DuaelFrI'm on it
Comment #79
DuaelFrComment #80
HazaIssues have been addressed, patch green and tested and already got RTBC before, so, let's go back to RTBC !
Comment #81
iMiksuI assume that in #80 the patch was tested and reviewed to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards?
However, we could use some screenshots here.
Comment #82
DuaelFrThat's a bit more than 80 chars :p
Adding screenshots and fixing this patch are novice tasks. Have fun mates!
Comment #83
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedWorking on it
Comment #84
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedAttaching screenshots before and after applying the patch
Comment #85
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedFixing 80 chars in the line as @DuaelFr suggested :)
Comment #86
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedComment #87
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedComment #89
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedRe-uploading .patch & interdiff, I missed some changes.
Sorry!
Comment #90
heykarthikwithuComment #91
DuaelFrThank you all for your help here! I hope we can see this one commited soon :)
Beware, the interdiff in #89 is wrong, though. I applied #79 then #89 to check differences and here is the right interdiff file.
Comment #92
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedOh! My apologizes and thanks @DuaelFr!
Comment #93
alexpottThis change is actually incorrect. hook_update_N function descriptions need to be on one line. And it is important because they are actually displayed to the user. Also I thought they are excluded from the 80 character limit but I can't find that documented on https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... so I think the 80 character limit applies.
How about
Add 'use_description_as_link_text' setting to file field formatters.
Also the second part of the file docblock probably could explain the choice of default values better than just say
This is done to ensure backward compatibility.
- imo that line should be removed and inline docs added to the function about why it's TRUE for file_default and FALSE for file_table.Comment #94
DuaelFrLet's try toget this one fixed before the summer ;)
@alexpott I made the changes you asked for, I hope my comments are not too bad now.
Comment #95
esolitosChanges are ok as comments are perfectly understandable, back to RTBC. :)
Comment #97
DuaelFrFixed tests now #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button is live.
Comment #99
DuaelFrLast patch was wrong but interdiff was good
Let's try again !
Comment #100
tstoecklerComment #101
tstoecklerBack to RTBC. Thanks, @DuaelFr!
Comment #102
DuaelFr\o/ Thank you for the reviews :)
Comment #103
larowlanThis is looking great, thanks - one think we need one minor change to the update path
if we change this from
type: mapping
totype: field.formatter.settings.file_default
then we don't need to duplicate the new property. Given they both are the same, makes sense I think.nit: should be blank line after break; - can be fixed on commit
I think we should keep track of which displays we actually made changes too and only call
::set
and::save
if we actually changed somethingshould be 'file descriptions' not 'files descriptions', can be fixed on commit
needs to use short array syntax now
seeing this 'read some yml, insert into config' pattern fairly regularly, we need a helper class I reckon, if someone feels like opening a follow-up task. Would be a class with static methods that took the connection as an argument.
Thanks again for the hard work here
Comment #104
DuaelFr#103 points 1 to 5 done
Comment #106
DuaelFrRerolled for D8.5
Comment #107
Wim LeersComment #108
DuaelFr❤️
Comment #109
larowlanAdding credits for reviews that helped shape the patch
Comment #110
larowlanFixed on commit
Committed as daf25aa and pushed to 8.5.x
Published change record