Problem/Motivation
While discussing the design for the Media Library, we noticed that the naming of the standard Media types is not very clear out-of-the-box. It is not automatically clear which file types can be added in the "File" media type. The supported filed extensions are "txt, rtf, doc, docx, ppt, pptx, xls, xlsx, pdf, odf, odg, odp, ods, odt, fodt, fods, fodp, fodg, key, numbers, pages". Media types should be as specific as possible to manage user expectations. The media type "File" seems to be too broad of a term for the limited list of file extensions.
Proposed resolution
Looking at the supported file extensions, "Document" seems to be a better fit for the "File" media type. This will only be done for new installs, existing sites should not be changed.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#26 | 3019202-18.patch | 23.79 KB | g_miric |
#23 | 3019202-17.patch | 23.79 KB | g_miric |
#21 | 3019202-16.patch | 21.73 KB | g_miric |
#20 | 3019202-15.patch | 19.73 KB | g_miric |
#14 | 3019202-14.patch | 23.99 KB | seanB |
Comments
Comment #2
seanBPatch is attached.
Comment #4
seanBThis should fix the tests.
Comment #6
seanB#4 had a random fail in
CKEditorIntegrationTest
(https://www.drupal.org/pift-ci-job/1144361) which does not seem to be related to the changes in this patch. Changed to needs review again.Comment #7
seanBThis issue has just been discussed in the UX call. The feedback was positive! I think we can land this as soon as possible.
Comment #8
marcoscanoWhile reviewing, at some point I started wondering what would happen with a hypothetical site that had already a
document
media type, but I couldn't come up with anything significant. The only thing I saw is that if the site enabled Media Library after this modification, they wouldn't get the media library default form and display config for thedocument
type, since there would be a dependency missing on the source field. But since all other custom media types would not get those configs either, I think this shouldn't be a big deal.+1 from me.
Thanks!
Comment #9
xjmHmm, this does sound like a bug to me.
How about just changing the user-facing labels, and leaving the machine names alone? Has that been discussed?
Comment #10
seanBWe have #2988433: Automatically create and configure Media Library view and form displays to fix that.
We thought about it. If we do that though, users trying to create a new media type names 'file' would not get the type ID 'file'. We are also only trying to do this for new sites in this patch. That's why we prefer consistency and it shouldn't be a problem to rename the type ID as well.
Comment #11
xjmMaybe we should postpone this on #2988433: Automatically create and configure Media Library view and form displays then? It would at least need an @todo, but postponing seems safer. Edit: Never mind, that one was committed now. :)
What exactly happens if someone tries to create
file
? Is it a validation error?file_2
or something? Either seems acceptable to me.FWIW changing optional config machine names is allowable in a minor I think (because it doesn't affect existing sites), but it is also a bit disruptive, which is why for other issues like this we've just changed the UI labels and had a followup for the machine names.
Comment #12
xjmIf we do change the machine names, we should probably add tests for the things described in #8 and maybe #10.
Comment #13
seanBThanks @xjm. In #2988433: Automatically create and configure Media Library view and form displays there is an update test to make sure existing media types will get the new config. This issue needs to be rerolled on top of that (tagging for that), but I think this case is already covered.
If you think we need additional tests, could you elaborate on which specific case(s) should be tested? I would be happy to write that.
Comment #14
seanBRerolled.
You get an error:
The machine-readable name is already in use. It must be unique.
, which would be weird if you don't have a media type called file. I think that is a good argument for changing the technical name in this patch directly.As mentioned before I think #8 / #10 are fixed and tested by #2988433: Automatically create and configure Media Library view and form displays. If there are specific extra tests that you would like to see, please let me know!
Comment #16
Wim LeersAFAICT this is ready.
Comment #17
phenaproximaI was going to suggest changing this to $assert_session->elementCount(), but that's a bit outside the scope of the patch. So, forget it. :)
Question: This is going from 'field_media_file' to 'field_media_document_file'. Why the extra verbosity? Is there any reason not to call the field 'field_media_document'?
This description reads strangely to me. Not sure what a better one would be, but we should take the time to improve it here. Maybe something like "An uploaded file or document, such a PDF"?
Comment #18
Wim LeersGood points, thanks Adam!
Comment #19
Wim LeersMarking
for points 2 and 3.Comment #20
g_miric CreditAttribution: g_miric at FFW, Develomon commentedImplemented changes for points 2 and 3.
Comment #21
g_miric CreditAttribution: g_miric at FFW, Develomon commentedAdded missing files.
Comment #22
seanBThe latest patch seems to delete a bunch of files instead of renaming them? Could you check the difference between patch #14 and #21?
Comment #23
g_miric CreditAttribution: g_miric at FFW, Develomon commentedComment #24
g_miric CreditAttribution: g_miric at FFW, Develomon commentedComment #25
phenaproximaOne small, but important change...then it's RTBC.
This needs to say "...such as a PDF." (It's currently missing the "as".)
Comment #26
g_miric CreditAttribution: g_miric at FFW, Develomon commentedComment #27
phenaproximaThanks! RTBC once testbot signs off.
Comment #28
Gábor HojtsySuperb, thanks everyone.
Comment #30
rosinegrean CreditAttribution: rosinegrean at PitechPlus commented