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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

seanB’s picture

Status: Active » Needs review
FileSize
18.04 KB

Patch is attached.

Status: Needs review » Needs work

The last submitted patch, 2: 3019202-2.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review
FileSize
18.24 KB
1.58 KB

This should fix the tests.

Status: Needs review » Needs work

The last submitted patch, 4: 3019202-4.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review

#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.

seanB’s picture

This issue has just been discussed in the UX call. The feedback was positive! I think we can land this as soon as possible.

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community

While 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 the document 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!

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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 the document type, since there would be a dependency missing on the source field.

Hmm, 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?

seanB’s picture

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 the document type, since there would be a dependency missing on the source field.

We have #2988433: Automatically create and configure Media Library view and form displays to fix that.

How about just changing the user-facing labels, and leaving the machine names alone? Has that been discussed?

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.

xjm’s picture

Maybe 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.

xjm’s picture

Issue tags: +Needs tests

If we do change the machine names, we should probably add tests for the things described in #8 and maybe #10.

seanB’s picture

Issue tags: +Needs reroll

Thanks @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.

seanB’s picture

Rerolled.

What exactly happens if someone tries to create file? Is it a validation error? file_2 or something? Either seems acceptable to me.

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!

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
  • Patch still applies.
  • Removing Needs tests per #14.
  • No update path because This will only be done for new installs, existing sites should not be changed. — which makes sense.

AFAICT this is ready.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
    @@ -122,10 +122,10 @@ public function testMediaDisplay() {
    +    $this->assertSame(1, count($page->findAll('css', 'article.media--type-document > div')));
    

    I was going to suggest changing this to $assert_session->elementCount(), but that's a bit outside the scope of the patch. So, forget it. :)

  2. +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.media.document.default.yml
    @@ -2,14 +2,14 @@ langcode: en
    -    - field.field.media.file.field_media_file
    -    - media.type.file
    +    - field.field.media.document.field_media_document_file
    

    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'?

  3. +++ b/core/profiles/standard/config/optional/media.type.document.yml
    @@ -1,13 +1,13 @@
    +description: "Use local documents for reusable media."
    

    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"?

Wim Leers’s picture

Good points, thanks Adam!

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +php-novice, +DevDaysCluj

Marking Needs work for points 2 and 3.

g_miric’s picture

Implemented changes for points 2 and 3.

g_miric’s picture

Added missing files.

seanB’s picture

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaStandardProfileTest.php
diff --git a/core/modules/media_library/config/optional/core.entity_form_display.media.file.media_library.yml b/core/modules/media_library/config/optional/core.entity_form_display.media.file.media_library.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/modules/media_library/config/optional/core.entity_view_display.media.file.media_library.yml b/core/modules/media_library/config/optional/core.entity_view_display.media.file.media_library.yml
deleted file mode 100644

+++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php
diff --git a/core/profiles/demo_umami/config/install/core.entity_form_display.media.file.default.yml b/core/profiles/demo_umami/config/install/core.entity_form_display.media.file.default.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/demo_umami/config/install/field.storage.media.field_media_file.yml b/core/profiles/demo_umami/config/install/field.storage.media.field_media_file.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/demo_umami/config/install/media.type.file.yml b/core/profiles/demo_umami/config/install/media.type.file.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml b/core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/standard/config/optional/core.entity_view_display.media.file.default.yml b/core/profiles/standard/config/optional/core.entity_view_display.media.file.default.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/standard/config/optional/field.field.media.file.field_media_file.yml b/core/profiles/standard/config/optional/field.field.media.file.field_media_file.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/standard/config/optional/field.storage.media.field_media_file.yml b/core/profiles/standard/config/optional/field.storage.media.field_media_file.yml
deleted file mode 100644

+++ /dev/null
diff --git a/core/profiles/standard/config/optional/media.type.file.yml b/core/profiles/standard/config/optional/media.type.file.yml
deleted file mode 100644

The latest patch seems to delete a bunch of files instead of renaming them? Could you check the difference between patch #14 and #21?

g_miric’s picture

g_miric’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

One small, but important change...then it's RTBC.

+++ b/core/profiles/demo_umami/config/install/media.type.document.yml
@@ -1,13 +1,13 @@
+description: "An uploaded file or document, such a PDF."

This needs to say "...such as a PDF." (It's currently missing the "as".)

g_miric’s picture

Status: Needs work » Needs review
FileSize
23.79 KB
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! RTBC once testbot signs off.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb, thanks everyone.

  • Gábor Hojtsy committed 5655082 on 8.8.x
    Issue #3019202 by g_miric, seanB, xjm, Wim Leers, phenaproxima,...
rosinegrean’s picture

Issue tags: -DevDaysCluj +DevDaysTransylvania

Status: Fixed » Closed (fixed)

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