Add "File" MediaSource plugin

Fixed typo from copy/pasting

What would be the best place for the document plugin to exist? Inside media_entity itself, as a submodule, or in a completely separate folder/namespace?

EDIT: The same question applies to the image plugin in #2831937: Add "Image" MediaSource plugin

untested WIP

Test steps:

0) Programmatically create a field to be used as source
1) Go to /admin/structure/media/add, make sure our type is available in the select
2) Create a bundle of our type
2.1) Fill in the label
2.2) Select our type on the drop down
2.3) (wait for ajax to finish, then) make sure the dropdown field "type_configuration[document][source_field]" is present
2.4) Make sure our meta-data fields are present (MIME and size)
2.5) Select our source field
2.6) Save the form
3) Make sure the current URL is /admin/structure/media
4) Check that our bundle is listed there
5) Go to admin/structure/media/manage/{bundle_name}/form-display, hide the media name from the form
6) Go to /media/add/{bundle_name}
7) Upload a file, save the form
8) Make sure the resulting URL is /media/{media_id}
9) Check whether the media name is the filename
10) Check that the thumbnail is there, using the generic icon. (an <img> is present, pointing to generic.png)

Would anyone suggest additional tests, or anything that could be abstracted into a common test base?

1) Go to /admin/structure/media/add, make sure our type is available in the select
2) Create a bundle of our type
2.1) Fill in the label
2.2) Select our type on the drop down
2.3) (wait for ajax to finish, then) make sure the dropdown field "type_configuration[document][source_field]" is present
2.4) Make sure our meta-data fields are present (MIME and size)
2.5) Save the form
3) Programmatically create two fields attached to this bundle:
- one field of a supported type
- one field of a non-supported type
4) Go to /admin/structure/media/manage/{bundle_name}
5) Make sure only the supported field type is shown in the dropdown
6) Save the form
7) Make sure the current URL is /admin/structure/media
8) Check that our bundle is listed there
9) Go to admin/structure/media/manage/{bundle_name}/form-display, hide the media name from the form
10) Go to /media/add/{bundle_name}
11) Upload a file, save the form
12) Make sure the resulting URL is /media/{media_id}
13) Check whether the media name is the filename
14) Check that the thumbnail is there, using the generic icon. (an <img> is present, pointing to generic.png)

now with full index line in the patch

Generally this looks good to me. Nice test coverage :) I have a few comments...

1. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Document.php
   @@ -0,0 +1,123 @@
   +      'mime' => $this->t('File MIME'),
   

   Can this be "MIME type"?

2. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Document.php
   @@ -0,0 +1,123 @@
   +        return !$file->filemime->isEmpty() ? $file->getMimeType() : FALSE;
   

   Can be return $file->getMimeType() ?: FALSE for brevity's sake.

3. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Document.php
   @@ -0,0 +1,123 @@
   +        $size = $file->getSize();
   +        return $size ? $size : FALSE;
   

   0 is a valid file size. Not sure why anyone would upload a 0-byte file, but it's a valid size. We might want to change the return check to is_numeric().

4. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Document.php
   @@ -0,0 +1,123 @@
   +      $thumbnail = $this->configFactory->get('media_entity.settings')->get('icon_base') . "/{$mimetype[0]}-{$mimetype[1]}.png";
   

   Let's put two dashes between $mimetype[0] and $mimetype[1], since either of those could contain dashes as a matter of course. For example, application/octet-stream will become application-octet-stream.png, which is slightly confusing; application--octet-stream.png is clearer.

5. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Document.php
   @@ -0,0 +1,123 @@
   +        $thumbnail = $this->configFactory->get('media_entity.settings')->get('icon_base') . "/{$mimetype[1]}.png";
   +
   +        if (!is_file($thumbnail)) {
   +          $thumbnail = $this->configFactory->get('media_entity.settings')->get('icon_base') . '/generic.png';
   

   Can we get the icon_base as a variable so that we don't need to keep calling the config factory?

6. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Document.php
   @@ -0,0 +1,123 @@
   +    else {
   +      $thumbnail = $this->configFactory->get('media_entity.settings')->get('icon_base') . '/generic.png';
   +    }
   

   This if structure could be flattened more, for clarity's sake.

7. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/DocumentTest.php
   @@ -0,0 +1,57 @@
   +  public function testDocumentType() {
   +
   +    $session = $this->getSession();
   

   Nit: There's an extra line of white space.

8. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/DocumentTest.php
   @@ -0,0 +1,57 @@
   +    $bundle_name = 'sfbhg6gfe54rdfjbhbf7';
   

   Let use use $this->randomMachineName(). ;)

9. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/DocumentTest.php
   @@ -0,0 +1,57 @@
   +    $page->attachFileToField("files[field_file1_0]", \Drupal::root() . '/core/misc/druplicon.png');
   

   I think \Drupal::root() is exposed as a property of the test class ($this->root?); it would be preferable to use that.

10. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/DocumentTest.php
    @@ -0,0 +1,57 @@
    +    $this->assertEquals($media->label(), 'druplicon.png');
    

    Can we test with an actual document instead of an image, just to avoid stepping on the image plugin's toes? Maybe a .txt or a .pdf would suffice here.

11. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/MediaTypeBaseTest.php
    @@ -0,0 +1,143 @@
    +class MediaTypeBase extends MediaEntityJavascriptTestBase {
    

    Should be MediaTypeTestBase.

12. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/MediaTypeBaseTest.php
    @@ -0,0 +1,143 @@
    +   *   The bundle type id.
    

    s/id/ID.

@phenaproxima thanks for the review!

Addressed all points in #14, except for:

#14-9: I couldn't find it, is it called something else perhaps?

This new patch also creates a default bundle for document. We are not yet including a test for that, because it will be better to wait until #2625854: Provide default source_field when creating new media entity bundles lands, so we can rely on the source field being automatically created.

This should deal with the testbot complain

testbot I still love you

For manual testing only (automated tests still failing, work in progress)

This patch needs to be applied after the patch #92 from #2831274: Bring Media entity module to core as Media module is applied.

This is a conversion of #23, renaming "Document" into "File", which can then be used as a generalization and be extended by the image handler, for instance.

Patches:
- interdiff: The interdiff with the previous patch
- full: Against 8.3.x HEAD (including #2831274-92) to trigger all drupal tests
- only-plugin: The real work done here, to be applied on the top of #2831274-92

1. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/File.php
   @@ -0,0 +1,121 @@
   + *   label = @Translation("File"),
   

   I'm wondering whether file should be document instead? File seems really generic as image is also a file.

2. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/File.php
   @@ -0,0 +1,121 @@
   +    $file = $media->{$source_field}->entity;
   ...
   +    $file = $media->{$source_field}->entity;
   ...
   +    if (!empty($source_field) && ($file = $media->{$source_field}->entity)) {
   

   Nitpick: Using $media->get($source_field) is easier to read IMHO

3. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/File.php
   @@ -0,0 +1,121 @@
   +    if ($file) {
   +      $mimetype = $file->getMimeType();
   +      $mimetype = explode('/', $mimetype);
   +      $thumbnail = $icon_base . "/{$mimetype[0]}--{$mimetype[1]}.png";
   +
   +      if (!is_file($thumbnail)) {
   +        $thumbnail = $icon_base . "/{$mimetype[1]}.png";
   +      }
   +    }
   +
   +    if (!is_file($thumbnail)) {
   +      $thumbnail = $icon_base . '/generic.png';
   +    }
   

   Maybe a few documentation words about the logic would be nice. Note: In an ideal world this method would have a corresponding kernel test

4. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/File.php
   @@ -0,0 +1,121 @@
   +    /** @var \Drupal\field\FieldConfigInterface $field */
   +    return $this->entityTypeManager
   +      ->getStorage('field_config')
   +      ->create([
   +        'field_storage' => $this->getSourceFieldStorage(),
   +        'bundle' => $bundle->id(),
   +      ]);
   

   $field doesn't exist here, so we could remove this line

5. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/FileTest.php
   @@ -0,0 +1,51 @@
   +    // Adjust the allowed extensions on the source field.
   +    \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
   +    $bundle->getType()
   +      ->getSourceField($bundle)
   +      ->setSetting('file_extensions', 'txt')
   +      ->save();
   

   That's the only non UI code in this test, I think we should use the UI for this change as well. Mixing UI and non UI code in tests is tricky to deal with

6. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/FileTest.php
   @@ -0,0 +1,51 @@
   +    $media = Media::load(1);
   +    $this->assertEquals($media->label(), 'README.txt');
   

   Nitpick: this should be the other way round, as $expected should be the first parameter

7. +++ b/core/modules/media_entity/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
   @@ -0,0 +1,97 @@
   +    // assertWaitOnAjaxRequest() doesn't work on the machine name element.
   +    $session->wait(5000, "jQuery('.machine-name-value').text() === '$bundle_name'");
   

   Do you mind opening up an issue for that? Do you know why this is the case? Note: There is \Drupal\FunctionalJavascriptTests\JavascriptTestBase::assertJsCondition for that kind of stuff as well

@marcoscano :
I just applied the media_entity to media renaming (This patch is based on media 2831274-99.patch),
and changes according to Daniel's comments #3, #4, #6

Hope it is fine.

Thanks Daniel for reviewing, and Philippe for starting to work on it.

The patch attached finishes the "renaming" process and also addresses most of the review in #26.

At this point the patches attached were both based on 2831274-99.

Some comments:

26-1: As commented with @dawehner on the sprint, the idea is that the Image handler extends this plugin, so this name is probably the most appropriate in this case. A "Document" plugin can be created also extending "File", if needed to expose specific UI improvements for documents.
26-2: As part of the work being done in the Image plugin, this is being refactored to use a getter in the base class.
26-3: OK, left the "ideal-world part" postponed until we get a little closer to that point :)
26-4: OK
26-5: OK
26-6: OK
26.7: Pending. I will ask @mtodor tomorrow about that because he is the one who whispered me this solution, so he may be able to help identifying the cause and filing an issue in that case.

NOTE: Sorry I am unable to provide a clean interdiff between 29 and 31 at this point. I had to update core due to the RevisionableEntityBundleInterface inclusion, and then messed up with the local branch I was working on.

testbot please do your work

Missed the renaming in two spots, corrected now.

Rebased after 2831274-120, and refactored accordingly.

Rebased and refactored on top of 2831274-136

Nice work! In my opinion, this is complete.
I have few nitpicks, but nothing crucial.

1. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,110 @@
   +    // The default name will be the filename of the source_field, if present.
   +    $file = $this->getSourceFieldValue($media);
   +    return $file->getFilename();
   

   Comment should be corrected, since source field has to be present. Also there is no need to create new variable ($file), everything can be one expression: return $this->getSourceFieldValue($media)->getFilename();

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
   @@ -0,0 +1,50 @@
   +    \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
   

   This functionality should be documented, it's not so obvious, why it's here.

I rebased this patch on 2831274-147. Also i fixed comments from #39 and added the entity display and form display.

1. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,109 @@
   +    // directory, based on the mime information. For instance, if an icon file
   

   Nit: 'mime' should be MIME.

2. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,109 @@
   +    // mime type.
   

   MIME

3. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,109 @@
   +  /**
   +   * Get source field file entity.
   +   *
   +   * @param \Drupal\media\MediaInterface $media
   +   *   The media entity.
   +   *
   +   * @return \Drupal\file\FileInterface
   +   *   The file entity present in the source field.
   +   */
   

   Missing @throws annotation.

4. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,109 @@
   +  protected function getSourceFieldValue(MediaInterface $media) {
   +    $source_field = $this->configuration['source_field'];
   +
   +    if (empty($source_field)) {
   +      throw new \RuntimeException('Source field for media file handler is not defined.');
   +    }
   

   I think this part of the method should be moved into MediaHandlerBase. It's too useful not to have that everywhere. Then this handler can override and extend the base implementation with its additional logic.

Rebased #40 to the latest media patch #2831274-176. Fixed comments 1/2/3 from #42.

The last comment is not fixed, the getSourceFieldValue is specific for file based plugins. It fetches the file entity from the source field. The handler base does contain a getSourceValue method to fetch the main property for the source field (I think that's the file ID). Both functions are useful but since they have similar names this might be confusing. To fix this I renamed the getSourceFieldValue function to getSourceFile.

Most media plugins probably won't have file entities in their source fields anyway, so I don't think this is an issue. I did however change the media patch and added the check to getSourceValue in the MediaHandlerBase.

Just saw the comment in https://www.drupal.org/node/2831937#comment-11829154 about the changed test. Updated so it will hopefully pass. Also moved some generic code needed by the file and image plugin tests to MediaHandlerTestBase.

Ok, forgot to rename something and there is also a function that could be removed since it is no longer used.

Looking good! just a couple thoughts

+++ b/core/modules/media/src/Plugin/media/Handler/File.php
@@ -0,0 +1,113 @@
+    if ($file) {
...
+      $thumbnail = $icon_base . "/{$mimetype[0]}--{$mimetype[1]}.png";
+
+      if (!is_file($thumbnail)) {
+        $thumbnail = $icon_base . "/{$mimetype[1]}.png";
+      }
+    }

For an example mime type of "image/jpeg" this code will look for jpeg.png and image--jpeg.png but not image.png. Is this intentional? Seems like having a common icon for all images would be common.

Also do we think this type of icon look up would be common? If so maybe we should have general function in MediaHandlerBase like
function getIconFile($parts, $fallback = 'generic.png')
This would handle looking for all possibilities and concat '--' etc.
Then could all it here like
$this->getIconFile(explode('/',$file->getMimeType())

+++ b/core/profiles/standard/config/optional/media.type.file.yml
@@ -0,0 +1,14 @@
+description: 'Use the "File" media type for uploading local files.'

Should this somehow explain to the user they will not get media type specific features? For instance if I use this handler to upload an image instead of the Image handler I won't get the image thumbnail, correct?

If you have media before this might be obvious but from the description it would be easy to assume you could use it for any local files and get the image thumbnails.

+++ b/core/modules/media/src/Plugin/media/Handler/File.php
@@ -0,0 +1,113 @@
+ *   description = @Translation("Provides business logic and metadata for local files and documents."),

Same comment about the description.

For an example mime type of "image/jpeg" this code will look for jpeg.png and image--jpeg.png but not image.png. Is this intentional? Seems like having a common icon for all images would be common.

This could/should probably be added. Will look at that.

Also do we think this type of icon look up would be common? If so maybe we should have general function in MediaHandlerBase

For now I think this is the only handler directly handling files and dealing with generic icons. The image handler uses the images for thumbnails and other handlers like youtube and instagram etc will also need to provide their own thumbnails. Since every handler should (imho) provide their own icon(s), it's probably best not to have this in MediaHandlerBase.

Should this somehow explain to the user they will not get media type specific features? For instance if I use this handler to upload an image instead of the Image handler I won't get the image thumbnail, correct?

I don't think this is supposed to explain anything about media type specific features. The file media type is supposed to replace the file fields in core. There is no real explanation on the difference between file fields and image fields. In the current situation you also won't get a thumbnail when uploading an image in a file field. If you have any suggestions on how to improve this please let me know :)

There is no real explanation on the difference between file fields and image fields.

Yup, is media worse in some way then the existing difference between file and image fields?

Yup, is media worse in some way then the existing difference between file and image fields?

That is good point. It is the same problem so need to hold up this issue.

Regarding my icon comments in #48, explanations in #49 and #50 make sense and because my suggestion could easily be added later.

So regarding my comments in #48 I think the current state of the patch is good.

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
@@ -0,0 +1,146 @@
+    $this->assertJsCondition("jQuery('.machine-name-value').text() === '$type_name'");

Do we actually have to test this? Isn't this standard core machine name element? If the machine name is not correct won't it fail later anyways in this function when we test for it's existence on /admin/structure/media

+++ b/core/modules/media/src/Plugin/media/Handler/File.php
@@ -0,0 +1,113 @@
+    if (empty($file)) {
+      throw new \RuntimeException('The source field does not contain a file entity.');
+    }

There is nothing to enforce that the File source field is required it doesn't make sense throw this expection if there is no source file.

I tested this and I can make the field not required. Then when I try to create a media entity without a file I get this hard error.

I typed an extensive review. Then lost most of it because Dreditor and Chrome. So this one is a bit more terse.

1. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   + * Provides the media handler plugin for Files.
   

   s/Files/File entities/

   I'd also expect an @see \Drupal\file\FileInterface to stress the relationship.

2. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   + *   description = @Translation("Provides business logic and metadata for local files and documents."),
   

   Omit "documents", because not relevant, nor mentioned anywhere.

   Omit "business logic", because it doesn't help at all at /admin/structure/media/add

   Suggestion:
   Allows local files to be used as media.

3. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +      'mime' => ['label' => $this->t('MIME type')],
   +      'size' => ['label' => $this->t('Size')],
   

   These keys should be constants on an interface.

4. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +      case 'mime':
   ...
   +      case 'size':
   
   +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
   @@ -0,0 +1,62 @@
   +    $provided_fields = [
   +      'mime',
   +      'size',
   +    ];
   

   These places would benefit from them being constants on an interface.

5. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +    $icon_base = $this->configFactory->get('media.settings')->get('icon_base');
   

   icon_base has been renamed in #2831274: Bring Media entity module to core as Media module. So this is broken now.

6. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +    // We try to magically use the most specific icon present in the $icon_base
   

   s/magically/automatically/

7. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +    // directory, based on the MIME information. For instance, if an icon file
   

   s/MIME information/MIME type/

8. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +    if ($file) {
   +      $mimetype = $file->getMimeType();
   +      $mimetype = explode('/', $mimetype);
   +      $thumbnail = $icon_base . "/{$mimetype[0]}--{$mimetype[1]}.png";
   +
   +      if (!is_file($thumbnail)) {
   +        $thumbnail = $icon_base . "/{$mimetype[1]}.png";
   +      }
   +    }
   +
   +    if (!is_file($thumbnail)) {
   +      $thumbnail = $icon_base . '/generic.png';
   +    }
   

   This needs explicit test coverage.

9. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
   @@ -0,0 +1,113 @@
   +    // The default name will be the filename of the source_field.
   +    return $this->getSourceFile($media)->getFilename();
   

   The description sounds strange. And can even be omitted, because it's so trivial.

10. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
    @@ -0,0 +1,113 @@
    +   * Get source field file entity.
    

    Gets File entity from given Media entity.

11. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
    @@ -0,0 +1,113 @@
    +   *   The media entity.
    

    s/The/A/

12. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
    @@ -0,0 +1,113 @@
    +   *   The file entity present in the source field.
    

    s/present in/referenced by/

13. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
    @@ -0,0 +1,113 @@
    +   *   If the source field for the handler is not defined, or if the
    +   *   source field does not contain a file entity.
    

    Nit: 80 cols.

14. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
    @@ -0,0 +1,113 @@
    +      throw new \RuntimeException('Source field for media file handler is not defined.');
    

    s/media file/file media/

    Test coverage is missing.

15. +++ b/core/modules/media/src/Plugin/media/Handler/File.php
    @@ -0,0 +1,113 @@
    +      throw new \RuntimeException('The source field does not contain a file entity.');
    

    The message doesn't make sense (as Ted apparently already indicated in #53).

    Test coverage is missing.

16. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    + * Tests the file handler plugin.
    

    "handler plugin" again. It's either "media handler plugin" or "media handler", but never "handler plugin".

    Again confirms my concerns in #2831274.

17. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +   * Just dummy test to check generic methods.
    

    Eh… That's not a good description.

18. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +  public function testMediaFilePlugin() {
    

    Let's at least be somewhat consistent: s/MediaFilePlugin/FileMediaHandler/

19. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    // Create image media handler.
    

    s/image/file/

20. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    // Create a supported and a non-supported field.
    +    $fields = [
    +      'field_string_mime' => 'string',
    +    ];
    +    $this->createMediaFields($fields, $type_name);
    

    1. Only occurrence of "supported" vs "non-supported" in all of the Media module. Could use a better explanation of what this means
    2. Only a single field is being created… so the comment doesn't make sense?

21. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    // Hide the media name to test default name generation.
    +    $this->hideMediaField('name', $type_name);
    

    Neither the comment nor the method name make sense. Rename the method to hideMediaFieldWidget and the comment can be removed :)

22. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    // Create a media item.
    

    "media item"… see discussion in #2831274.

23. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    $assert_session->addressEquals('media/1');
    

    Didn't know this assertion method, cool, thanks :)

24. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    // Make sure the thumbnail is created.
    

    s/created/displayed/

25. +++ b/core/modules/media/tests/src/FunctionalJavascript/FileTest.php
    @@ -0,0 +1,62 @@
    +    // Load the media and check that all fields are properly populated.
    +    $media = Media::load(1);
    +    $this->assertEquals('README.txt', $media->label());
    +    $this->assertEquals('text/plain', $media->get('field_string_mime')->value);
    

    What about the size field?

26. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    + * A base test class for plugin types.
    

    This comment doesn't make sense.

    - media types?
    - media handlers?
    - media handler plugins?

    I've seen it all, and now "plugin types". This really points to how problematic the current naming is, i.e. how problematic "media handler" as a plugin type is.

27. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +   * Create storage and field instance, attached to a given media type.
    

    Nit: s/Create/Creates/

28. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +   *   The media type machine name.
    ...
    +   *   The type machine name.
    ...
    +   *   The media type machine name.
    ...
    +   *   The type machine name.
    

    Even these are inconsistent… They should all be:

    media type config entity type ID
    

29. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +    $config = FieldConfig::create([
    ...
    +    $config->save();
    

    Nit: Omit $config, it's not used anywhere.

30. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +    // Make the field visible in the form display.
    

    s/field/field widget/

31. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +   * Helper to create a set of fields in a media type.
    ...
    +   * Helper to test a generic type (bundle) creation.
    

    "Helper to" are not acceptable test method descriptions.

32. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +   * Hide a component from the default form display config.
    

    Nit:
    s/hide/Hides/
    s/from/in/

33. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +  public function createMediaTypeTest($type_name, $type_id, array $provided_fields = []) {
    

    The Test suffix in this helper method is very strange/unnecessary/

34. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +    $this->assertJsCondition("jQuery('.machine-name-value').text() === '$type_name'");
    

    Nit: Is there really only one with this class here?

    And why does this use jQuery?

35. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    @@ -0,0 +1,123 @@
    +    // Make sure the type is available.
    

    Again further indication that "media type" config entity is almost always the same as the corresponding "media handler" plugin. Because this is not testing a type like the comment says, but a handler.

    If even test authors are confusing this, what will it be like for end users?

36. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaHandlerTestBase.php
    --- /dev/null
    +++ b/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
    --- /dev/null
    +++ b/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
    --- /dev/null
    +++ b/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
    --- /dev/null
    +++ b/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
    --- /dev/null
    +++ b/core/profiles/standard/config/optional/media.type.file.yml
    

    Huh? This is modifying the Standard install profile?

    This doesn't make sense for two reasons:

    1. This should not yet be modifying the Standard install profile, that can only be done in 8.4, after this is proven in 8.3.
    2. What about existing sites adopting this? They'd have to figure all this out by themselves.

    I expected this to live in the non-optional configuration of the media module.

I have prove of #54.5 being broken: it points to http://d8/sites/default/files/styles/thumbnail/public//generic.png?itok=hWcY1sS1, and hence looks like this:

Also, the issue summary explains #54.2:

Adopt document file plugin

Adopt document file media type plugin handler

And it again confirms the confusion/complexity/strangeness around "handler" vs "plugin" etc, that I mentioned numerous times in #54:

media type plugin handler

So it seems to have first been called "plugin" or "plugin handler", then just "handler". But it really is still a plugin, so this notation is wrong, so it's actually a "'bla bla handler' plugin".

And even the prefix for "handler" cannot be agreed upon: "media type" is written here, but "media" is what is in the actual annotation.

Hence this is a critical thing to fix before this or #2831274 are committed to core.

Also see #2831274-206: Bring Media entity module to core as Media module, which I was able to write thanks to reviewing this issue.

Thank you all reviewers.The patch in this issue is not in sync with the latest patch in #2831274: Bring Media entity module to core as Media module Postponing this on #2831274: Bring Media entity module to core as Media module so that there would not be back-forth b/w issues.

//Naveen

Keeping up with API changes at #2831274-246: Bring Media entity module to core as Media module.

Keeping up with API changes at #2831274-248: Bring Media entity module to core as Media module.

Keeping up with API changes at #2831274-253: Bring Media entity module to core as Media module.

Keeping up with API changes at

Keeping up with API changes at #2831274-286: Bring Media entity module to core as Media module.

Working on implementing API changes of #2831274: Bring Media entity module to core as Media module

This one is based on #332 of #2831274: Bring Media entity module to core as Media module. Just uploading for now, will look at the reviews later!

So I just addressed some of the comments from #54 that were not done yet. Still based on #332 of #2831274: Bring Media entity module to core as Media module but should work with #336 as well.

Still todo:
#54.8
#54.33
#54.34
#54.36

Done:
#54.6 Done
#54.7 Done
#54.8 TODO
#54.17 Done
#54.20 Done
#54.21 Done
#54.24 Done
#54.25 Done
#54.28 Done, renamed type name / machine name everywhere to type id.
#54.29 Done
#54.30 Done
#54.31 Done
#54.32 Done
#54.33 Yes, but since the method already exists in MediaFunctionalTestCreateMediaTypeTrait we can just remove it. Any suggestions?
#54.34 TODO
#54.35 This is now called source, so I guess this is no longer an issue?
#54.36 Excellent point, I want to move it but maybe marcoscano can comment on the reason to do this?

Added missing patches...

Sorry, 54.2 is also still TODO!

Re #54.36, if I'm not mistaken the original idea was to follow more or less what the standard profile assumes for nodes.
In other words, just as the standard profile creates 2 content types, some fields, view/form displays, etc, the equivalent for media would also be created there. This would mean that sites that want to use media but not necessarily use these source plugins / fields / etc, wouldn't have them created just by enabling the module.

It is true that the upgrade paths would need to take care of, at least:
- Existing sites with the media_entity contrib module
- Existing sites without the media_entity contrib module, that upgraded core and then enabled the media module

There is probably some room for more discussion here, to look for alternatives that would better deal with all these scenarios.

It's really nice to see media into core is moving forward! ::thumbsup::

I did quick check and found two things that would be nice to have.

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,98 @@
   +    $mimetype = $file->getMimeType();
   +    $mimetype = explode('/', $mimetype);
   +
   +    $icon_names = array(
   +      $mimetype[0] . '--' . $mimetype[1],
   +      $mimetype[1],
   +      $mimetype[0],
   +    );
   +    foreach ($icon_names as $icon_name) {
   +      $thumbnail = $icon_base . '/' . $icon_name . '.png';
   +      if (is_file($thumbnail)) {
   +        return $thumbnail;
   +      }
   +    }
   

   I like this functionality and it would be really nice to add test that covers it (maybe there is already test for it, forgive me if I have missed it). Maybe it's possible just to clone generic.png into 'text-plain.png' and check is that icon used for new README.txt media entity.

2. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,98 @@
   +    return $icon_base . '/generic.png';
   

   This should not be used, instead fallback should be on parent, where annotation will be useddefault_thumbnail_filename = "generic.png"

54.2 Done
54.8 Done
54.34 Removed as suggested in #53
54.36 We discussed this today. As marcoscano already mentioned providing configuration for the media types is not something you should always enable. The standard profile is probably the best place.

#72.1 Done
#72.2 Done

Verifying that my #54 review is addressed. All points are addressed, except:

• #54.3 (constants)
• #54.33 ("Test" suffix in method name)
• #54.34 (use of jQuery in JS-based test assertion)
• #54.36 (modifying Standard install profile)

I'm explicitly reviewing the remarks in #54 that requested test coverage separately:

• #54.8 test coverage was added in #73.
• #54.14 + #54.15 test coverage is no longer relevant: that code no longer exists in the File media source plugin thanks to architectural improvements in #2831274: Bring Media entity module to core as Media module :)

#71: RE: #54.36: Aha, thanks for clarifying :) However, this would AFAICT be the first and only case of configuration in the Standard install profile that is configuration that is optional. Apparently there already is optional configuration in the Standard install profile: image styles and responsive image styles. So this addresses #54.36.1.
However, that still leaves the concern I raised in #54.36.2 for existing sites. I don't see why it's preferable to have this optional configuration in the Standard install profile instead of in the media module. This definitely needs further discussion.
To be clear: I am okay with the current approach, I'm only saying we should consciously weigh "optional in Standard" vs "optional in media".

#72.1: hah, that's also what I asked in #54.8 :)
#72.2: agreed.

And finally, a complete review:

1. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -36,6 +36,10 @@ media.type.*:
   +  label: 'File handler configuration'
   

   '"File" media source configuration'
   

2. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,99 @@
   + *   description = @Translation("Allows local files to be used as media."),
   

   As of #2831274-351: Bring Media entity module to core as Media module, the Media module's description is no longer Allows for media to be created. but Create reusable media.. @yoroy decided this was much better (and I think everyone will agree).

   We should update this accordingly.

   Use local files for reusable media.

   seems the logical consequence.

3. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,99 @@
   +      'mime' => $this->t('MIME type'),
   +      'size' => $this->t('Size'),
   

   Still should be class constants, as I said in #54.3.

4. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,99 @@
   +    $icon_base = $this->configFactory->get('media.settings')->get('icon_base_uri');
   

   This is no longer used, can be removed.

5. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,99 @@
   +    if (!$file) {
   +      return parent::getMetadata($media, $name);
   +    }
   

   We need a comment here: when can this happen?

6. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,99 @@
   +    switch ($name) {
   +
   +      case 'default_name':
   +        return $file->getFilename();
   +
   +      case 'thumbnail_uri':
   +        return $this->getThumbnail($file) ?: parent::getMetadata($media, $name);
   +
   +      case 'mime':
   +        return $file->getMimeType() ?: FALSE;
   +
   +      case 'size':
   +        $size = $file->getSize();
   +        return is_numeric($size) ? $size : FALSE;
   +
   +      default:
   +        return parent::getMetadata($media, $name);
   +
   +    }
   

   Nit: I think all the newlines here are kind of atypical: we don't have any \n in most switch statements.

7. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php
   @@ -0,0 +1,75 @@
   +    // Create file media source.
   +    $this->createMediaTypeTest($type_id, 'file', $provided_fields);
   

   So are we creating a media source or a media type? Media type. Let's remove this comment.

8. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php
   @@ -0,0 +1,75 @@
   +    // Create a custom field for the media type to store the mime metadata
   +    // attribute.
   +    $fields = [
   +      'field_string_mime' => 'string',
   +      'field_string_size' => 'string',
   +    ];
   

   s/mime/MIME/
   s/attribute/attributes/

9. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php
   @@ -0,0 +1,75 @@
   +    // Test the mime type icon.
   

   s/mime/MIME/

10. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +   * @param string $field_type
    +   *   The field storage type.
    

    field type !== field storage type

11. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +   * @param string $type_id
    +   *   The media type config entity ID.
    ...
    +   * @param string $type_id
    +   *   The media type config entity ID.
    ...
    +   * @param string $type_id
    +   *   The media type config entity ID.
    ...
    +   * @param string $type_id
    +   *   The media type config entity ID.
    

    $media_type_id would be a better name than $type_id.

12. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +    // Make the field widget visible in the form display.
    +    $component = \Drupal::service('plugin.manager.field.widget')
    +      ->prepareConfiguration($field_type, []);
    +
    +    /** @var \Drupal\Core\Entity\Display\EntityFormDisplayInterface $entity_form_display */
    +    $entity_form_display = \Drupal::entityTypeManager()
    +      ->getStorage('entity_form_display')
    +      ->load('media.' . $type_id . '.default');
    

    The \n there makes it seem as if the comment applies to the first two lines. But it actually applies to everything I quoted, and more (i.e. several lines thereafter). Let's remove the \n for clarity.

13. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +      $entity_form_display = EntityFormDisplay::create(array(
    

    s/array()/[]/

14. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +   *   The type created.
    

    The created media type.

15. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +  public function createMediaTypeTest($type_id, $source_id, array $provided_fields = []) {
    

    So I already mentioned this Test suffix in #54.3. It's still here.

    Turns out it's here for good reason: it's also testing this!

    So then I would suggest doTestCreateMediaType() as the method name. This would be consistent with for example \Drupal\block_content\Tests\BlockContentTranslationUITest::doTestBasicTranslation().

16. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
    @@ -0,0 +1,138 @@
    +    // Save the page to create the type.
    

    s/page/form/

17. +++ b/core/profiles/standard/config/optional/field.field.media.file.field_media_file.yml
    @@ -0,0 +1,29 @@
    +  file_directory: '[date:custom:Y]-[date:custom:m]'
    ...
    +  max_filesize: ''
    

    This matches what \Drupal\file\Plugin\Field\FieldType\FileItem::defaultFieldSettings() has, great!

18. +++ b/core/profiles/standard/config/optional/field.field.media.file.field_media_file.yml
    @@ -0,0 +1,29 @@
    +  file_extensions: 'txt pdf'
    

    This is a superset of \Drupal\file\Plugin\Field\FieldType\FileItem::defaultFieldSettings() — this adds pdf.

19. +++ b/core/profiles/standard/config/optional/media.type.file.yml
    @@ -0,0 +1,14 @@
    +description: 'Use the "File" media type for uploading local files.'
    

    "uploading local files" sounds fairly strange.

    What about:

    'Use the "File" media type for storing media locally.'
    

Done.

#74.6 I've removed a newline before the first case and after default: but removing all newlines breaking the code standards. Therefore core switch statements are 50/50 with/without newlines after break;. Even though we have a return statements it is doing break; implicitly.
So probably we should leave \n there.

My bad 2831936-75-full.patch haven't been a -full actually.
Correct set of patches.

Awesome work l0ke, thnx!

#54.36

However, that still leaves the concern I raised in #54.36.2 for existing sites. I don't see why it's preferable to have this optional configuration in the Standard install profile instead of in the media module. This definitely needs further discussion.

So when you have an new/existing site with a minimal install, we decided you shouldn't get our default configuration. Same as you don't get page/article node types when you install. You could argue that this is a different situation and I personally do not have a strong opinion on this. But we kind of compared our situation to the node types.

#54.34 The jQuery test for the machine name is also used in Drupal\FunctionalJavascriptTests\Core\MachineNameTest. Since it is tested there we might just remove this? This is in the main core patch too btw #2831274: Bring Media entity module to core as Media module

#74.5 I told l0ke this shouldn't happen and he could just remove it, but in theory you could create your own field to a media type that is not required. So maybe it better to add a comment.

#74.6 l0ke already commented on this. I think it's fine now.

So I think this leaves us with:
- #54.34: Should we just remove the machine name test here?
- #74.5: Move back the check for $file and add a comment.

Thank you @seanB!

#74.5 Put condition back and add the comment.
#77 Add missing MediaSourceInterface import where used.

#2831274: Bring Media entity module to core as Media module has landed!

As laid out in #2825215: Media initiative: Roadmap, this issue is now the top priority for the Media team.

Just realized that this will likely need a reroll to account for architectural changes in Media.

Here is the reroll

Re #54.34
Since it is already tested in Drupal\FunctionalJavascriptTests\Core\MachineNameTest , i think it's fine if we remove that line. We are basically just using core functionality which is already tested in core.
But still, we have to wait until machine name is set. assertJsCondition does that implicitly.

As far as i see, all other comments are addressed, or is the currently something else open?

No longer postponed :)

As far as i see, all other comments are addressed, or is the currently something else open?

Indeed! So going over the patch once more…

+++ b/core/modules/media/src/Plugin/media/Source/File.php
@@ -0,0 +1,98 @@
+    // If field media type is not required.

I think this meant to say:

// If the source field is not required, it may be empty.

That's literally the only thing, and can clearly be fixed on commit.

+++ b/core/modules/media/src/MediaSourceInterface.php
@@ -71,6 +71,16 @@
+  /**
+   * Key for MIME type metadata attribute.
+   */
+  const METADATA_ATTRIBUTE_MIME = 'mime';
+
+  /**
+   * Key for size metadata attribute.
+   */
+  const METADATA_ATTRIBUTE_SIZE = 'size';
+

These do not belong on MediaSourceInterface. They are specific to files. I agree that they should be constants, but they should be constants of the File source, not the generic MediaSourceInterface. Can we move these?

#88 was surfaced in IRC by @phenaproxima, and I agreed:

18:04:02 <phenaproxima> I actually question a few things about that patch
18:04:04 <phenaproxima> For example
18:04:14 <seanB> WimLeers: Maybe that's too much to ask for one day though!
18:04:20 <phenaproxima> I don't know why the constants are on MediaSourceInterface...was that explained somewhere?
18:05:23 <phenaproxima> seanB: Any thoughts on that? ^^
18:06:00 <seanB> phenaproxima: See 54.3
18:06:09 <seanB> WimLeers made me do it :p
18:06:28 <phenaproxima> In that case, I am not sure I agree with WimLeers' conclusion :)
18:06:40 <phenaproxima> I mean, I'm OK with them being interface constants. But not MediaSourceInterface.
18:06:59 <phenaproxima> Those attributes are specific to files. MediaSouceInterface is not.
18:07:08 <phenaproxima> I suggest a new interface -- FileMediaSourceInterface.
18:07:11 <phenaproxima> Throw the constants on there.
18:07:26 <phenaproxima> WimLeers: ^^
18:08:02 <seanB> phenaproxima: Well the image source uses them too
18:08:11 <phenaproxima> seanB: Yes...and it extends File.
18:08:30 <phenaproxima> seanB: I'm *certain* we do not want those constants on MediaSourceInterface.
18:08:39 <seanB> phenaproxima: Well maybe we can just put them in Drupal\media\Plugin\media\Source\File
18:08:39 <WimLeers> yep good point
18:08:47 <WimLeers> yep that too
18:08:57 <phenaproxima> seanB: I'd rather put them in File instead of an interface
18:08:58 <WimLeers> do I unRTBC or will you?
18:09:03 <phenaproxima> WimLeers: I'll do it.
18:09:03 <seanB> phenaproxima: I agree with not putting them in MediaSourceInterface
18:09:05 <WimLeers> I think putting them in File makes sense
18:09:14 <seanB> I'll fix it now
18:09:16 <WimLeers> I missed that — glad you're double-checking my RTBCs :D
18:09:26 <WimLeers> also
18:09:32 <WimLeers> I love how I nitpicked that patch
18:09:34 <timplunkett> if you extend the class that it lives on, you can do static::METADATA_ATTRIBUTE_SIZE
18:09:36 <WimLeers> yet am still out-nitpicked
18:09:44 <WimLeers> timplunkett: yep!
18:09:46 <seanB> OMG it's so weird I don't have to apply patchzilla first
18:09:47 <WimLeers> Image can use it too then
18:09:50 <timplunkett> nitpick-topped!
18:09:51 <WimLeers> seanB: :D
18:09:55 <WimLeers> timplunkett: :D
18:10:26 <phenaproxima> seanB, WimLeers: NWed.

Fixed the comments in #85 and #88. Please review!

Nice. That looks perfect. I'd still like to do a complete read-through of the patch, but I doubt I'll find anything (it passed the @Wim Leers test, after all). Should be able to get to that today, then will restore RTBC if all looks good.

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,108 @@
   +use Drupal\media\MediaSourceInterface;
   

   I don't think this is used...

2. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,108 @@
   +  /**
   +   * Key for MIME type metadata attribute.
   +   */
   +  const METADATA_ATTRIBUTE_MIME = 'mime';
   +
   +  /**
   +   * Key for size metadata attribute.
   +   */
   +  const METADATA_ATTRIBUTE_SIZE = 'size';
   

   I think both of these need @var string annotations.

3. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,108 @@
   +        return $file->getMimeType() ?: FALSE;
   

   The documentation for MediaSourceInterface::getMetadata() dictates that NULL, not FALSE, should be returned if there is no value. https://api.drupal.org/api/drupal/core%21modules%21media%21src%21MediaSo...

4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
   @@ -0,0 +1,138 @@
   +    $entity_form_display = \Drupal::entityTypeManager()
   +      ->getStorage('entity_form_display')
   +      ->load('media.' . $media_type_id . '.default');
   +    if (!$entity_form_display) {
   +      $entity_form_display = EntityFormDisplay::create([
   +        'targetEntityType' => 'media',
   +        'bundle' => $media_type_id,
   +        'mode' => 'default',
   +        'status' => TRUE,
   +      ]);
   

   We could just use entity_get_form_display() here.

5. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
   @@ -0,0 +1,138 @@
   +    /** @var \Drupal\Core\Entity\Display\EntityFormDisplayInterface $entity_form_display */
   +    $entity_form_display = \Drupal::entityTypeManager()
   +      ->getStorage('entity_form_display')
   +      ->load('media.' . $media_type_id . '.default');
   +    $entity_form_display->removeComponent($field_name)->save();
   

   Should we use entity_get_form_display() here and only save the changes if the display is not new?

6. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
   @@ -0,0 +1,138 @@
   +    $this->assertSession()->fieldExists('Media source');
   +    $this->assertSession()->optionExists('Media source', $source_id);
   

   These should use $assert_session.

7. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
   @@ -0,0 +1,138 @@
   +    if (!empty($provided_fields)) {
   +      foreach ($provided_fields as $provided_field) {
   +        $assert_session->selectExists("field_map[{$provided_field}]");
   +      }
   +    }
   

   Nit: We don't need the if (!empty()) check. If $provided_fields is empty, the foreach will just be skipped anyway.

8. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
   --- /dev/null
   +++ b/core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml
   

   I'm not sure what's gained by adding optional config to standard. Standard does not include Media, so unless I'm missing something, this will never be installed...?

Fixed 93.1, 93.2 and 93.3
Not sure about 4 and 5 because entity_get_form_display is deprecated
Fixed 93.6, 93.7

93.8 If you install media installation, all the configs will be installed automatically. So the optional config makes totally sense.

Not sure about 4 and 5 because entity_get_form_display is deprecated

They were deprecated without anything to replace them. I have a patch (stalled) that moves their functionality into the entity display repository service and turns them into wrappers around those methods: #2367933: Move entity_get_(form_)display() to the entity display repository. Until that lands, it's OK to use those functions. :)

Ok, adjusted comments 4 and 5 form #93

My feedback, she is addressed. Restoring RTBC!

Just installed media module and tried to create a media type and found that it is not possible without this issue. Happy to see its already RTBC. By the look of it, without this patch, we wouldn't able to use media module at all(?).

I did a manual test with the patch in #96 and here is some findings:

1. "Field with source information" displaying filed's machine name(e.g. media.field_media_file)? (ref: screenshot)
2. Adding new media type doesn't allow to edit the allowed extension settings in file field as it's 'locked'(ref: screenshot 1, screenshot 2)

Am I missing something or is it by design?

"Field with source information" displaying filed's machine name(e.g. media.field_media_file)?

That's not exactly intentional, but it is known. The reason is that the options you see are the available field storages, and that is how they label themselves ($field_storage->label()).

Adding new media type doesn't allow to edit the allowed extension settings in file field as it's 'locked'

The field is intentionally locked, but if it means you can't change entirely reasonable settings, that is a problem. The only reason we need the field to be locked is so that it cannot be deleted through the UI, which would break the media type entirely. This is probably worth filing a related issue about.

thanks for the quick reply @phenaproxima.

The field is intentionally locked, but if it means you can't change entirely reasonable settings, that is a problem. The only reason we need the field to be locked is so that it cannot be deleted through the UI, which would break the media type entirely. This is probably worth filing a related issue about.

Right. the field edit tab holds settings like "Allowed file extensions", directory etc. As you can see currently the field supports only txt extension. We can have follow up to discuss/implement option to edit settings (without delete option), but at least we should allow common extensions (png, gif, jpg, jpeg, pdf) for this issue IMO.

I agree...but let's not support image extensions in this issue. We'll do that in #2831937: Add "Image" MediaSource plugin, which directly extends the work being done here.

I propose we preconfigure the field with txt, doc, docx, and pdf extensions. So I'm kicking this back for that (with tests).

I gave #101 an attempt. Not sure if you accept patches from strangers, but I was bored and if you aren't, i'm sorry!

Thanks, @johndevman! We accept patches from anybody. Drupal is not a closed-door effort, and the Media initiative will take all the help it can get!

The patch in #102 looks damn good to me. A few requests:

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -110,2 +111,20 @@
   +    $storage = $this->getSourceFieldStorage() ?: $this->createSourceFieldStorage();
   +    return $this->entityTypeManager
   +      ->getStorage('field_config')
   +      ->create([
   +        'field_storage' => $storage,
   +        'bundle' => $type->id(),
   +        'label' => $this->pluginDefinition['label'],
   +        'required' => TRUE,
   +        'settings' => [
   +          'file_extensions' => 'txt doc docx pdf'
   +        ]
   +      ]);
   

   Rather than copy all the code from MediaSourceBase::createSourceField(), we can just do this:

   return parent::createSourceField($type)->set('settings', ['file_extensions' => 'txt doc docx pdf']);

2. +++ b/core/modules/media/tests/src/Kernel/MediaSourceFileTest.php
   @@ -0,0 +1,52 @@
   +class MediaSourceFileTest extends MediaKernelTestBase {
   +  /**
   +   * Tests the file extension constraint.
   +   */
   +  public function testFileExtensionConstraint() {
   

   Nit: There needs to be a blank line after the opening brace of the class.

3. +++ b/core/modules/media/tests/src/Kernel/MediaSourceFileTest.php
   @@ -0,0 +1,52 @@
   +    $this->assertEquals('Only files with the following extensions are allowed: <em class="placeholder">txt doc docx pdf</em>.', (string) $result->get(0)->getMessage());
   

   Asserting the placeholder HTML and actual list of extensions is a little overzealous, IMHO; let's loosen this a bit to:

   $this->assertContains('Only files with the following extensions are allowed:', (string) $result->get(0)->getMessage());

4. +++ b/core/modules/media/tests/src/Kernel/MediaSourceFileTest.php
   @@ -0,0 +1,52 @@
   +  }
   +}
   \ No newline at end of file
    
   

   Nit: There should be a blank line before the closing brace of the class, and we'll need a new line at the end of the file so Git doesn't complain. :)

Fixed all items in #105

Sorry I don't want to put it back to N/W but requesting few changes

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,119 @@
   +    $icon_names = array(
   

   While we're on this, please replace it short array syntax

2. +++ b/core/modules/media/tests/src/Kernel/MediaSourceFileTest.php
   @@ -0,0 +1,73 @@
   +  protected function generateMedia($filename) {
   

   Move this method to MediaKernelTestBase so that we could re-use it for other(Image) MediaSource Plugins

3. +++ b/core/modules/media/tests/src/Kernel/MediaSourceFileTest.php
   @@ -0,0 +1,73 @@
   +            'test.patch' => str_repeat('a', 3000),
   

   test.patch should be $filename so that we could reuse it for "test.patch" and "test.txt"

I think that expanding the options for now is good enough. I do think we need to re-evaluate the locking of fields. For file/image fields changing the settings is not a problem. A entity reference source sometimes doesn't want to allow you to change the entity types and bundles you want to reference. Deleting a source field for a media type should never be allowed.

How about:

• Never allowing a field to be deleted as long is it's the source field of a media type.
• Making the field locking optional through a annotation in the media source. Storage is automatically locked when data is created.

Not sure what the best way is to stop a field from being deleted, but always locking all settings is obviously not what we should want.

#99:

The only reason we need the field to be locked is so that it cannot be deleted through the UI, which would break the media type entirely. This is probably worth filing a related issue about.

Hm, but I remember from the DrupalDevDays Seville reviews/analyses/discussions about Media that locking those field definitions was crucial. So I'm very much hoping we won't have to change that decision.

#98 + #100 + #101: hm, how come this is only allowing txt? Earlier, the patch was allowing txt pdf in field.field.media.file.field_media_file.yml, but apparently we somehow lost that?

#102 + #105: hm, we didn't need that before, it's a bit unfortunate that we now apparently need to override createSourceField(). But apparently this is something that was overlooked all this time?

#109:

For file/image fields changing the settings is not a problem.

Well, not having it be locked also means it can be deleted? And only some settings can be safely modified, IIRC?

but always locking all settings is obviously not what we should want.

Indeed, but so then the real question IMO is: how do we granularly lock settings?

#110 We do have 'MediaSourceFieldConstraintsInterface' interface which allows us to add constrains to the source field, but Implementing that interface seems redundant to me at a first glance for this source, as the constraint for file extension is already supported by the source field itself. I have not been around during the discussion of those interfaces so I'm not sure though.

--- b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php
+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php

@@ -104,7 +104,8 @@
-    $this->assertJsCondition("jQuery('.machine-name-value').text() === '{$media_type_id}'");
+    $this->getSession()
+      ->wait(5000, "jQuery('.machine-name-value').text() === '{$media_type_id}'");

Just saw this is also in MediaUiJavascriptTest. We should probably change it there as well. Or not change it here, don't have a strong opinion on that

#110

Hm, but I remember from the DrupalDevDays Seville reviews/analyses/discussions about Media that locking those field definitions was crucial. So I'm very much hoping we won't have to change that decision.

The base for this whole discussion was #291 and the solution summarized in #294 of the main patch issue. I think the solution I proposed (optional field locking and not allowing deletes for source fields) could be valid. Depends on if we can stop users from deleting a source field. But maybe stopping fields from being deleted is a separate issue which should be solved more generic (if not possible yet).

it's a bit unfortunate that we now apparently need to override createSourceField(). But apparently this is something that was overlooked all this time?

I don't think we overlooked this. It was discussed in the main patch, and I mentioned this in #294 and #295. Not sure if this is a bad thing, every media source is supposed to create a source field.

but so then the real question IMO is: how do we granularly lock settings?

I think what I proposed above is still valid here. We could optionally lock a field and find a way to stop fields from being deleted. For file and image fields you can safely change everything (that's why it came up so late), but for entity reference source fields it could lead to issues. But allowing granular control over locked field settings sounds pretty nice and could definitely work as well.

I personally don't know the intricacies of field configurations well enough to approve or disapprove this approach. I'm mainly trying to ask critical questions that hopefully contribute to get us to a good place :) I think this definitely needs further discussion with the right people before it can be RTBC again.

Maybe we should first allow some sane defaults for file extensions in this patch and discuss when and how to lock fields in a separate issue?

Yes, i agree. We have "txt doc docx pdf" as defaults. Which is ok for now. I would also propose to move the discussion about field locking into a separate issue. These discussion should not block this issue.

FYI: I removed the locking in #2831274-161 and implemented hook_entity_access to prevent deleting a source field. Not sure when and why this was removed.

Fixed all the comments from #107

+++ b/core/modules/media/src/Plugin/media/Source/File.php
@@ -0,0 +1,119 @@
+  public function getMetadata(MediaInterface $media, $name) {

describe the argument name to what it is. Change the argument name $name to $attribute_name

We already have an issue where the locked field could be altered via UI #2274433: Do not allow to alter Locked field via UI

Yes, i agree. We have "txt doc docx pdf" as defaults. Which is ok for now. I would also propose to move the discussion about field locking into a separate issue. These discussion should not block this issue.

Amen on having the txt, doc, pdf as defaults.

//Naveen

thanks @chr.fritsch. interdiff looks good.

+++ b/core/modules/media/tests/src/Kernel/MediaSourceFileTest.php
@@ -17,57 +13,18 @@
-    $this->assertEquals('field_media_file.0', $result->get(0)->getPropertyPath());
-    $this->assertNotContains('Only files with the following extensions are allowed:', (string) $result->get(0)->getMessage());

Looks like we lost all assert lines(except the count). Is it intentional?

Yes, because the second $media->validate() returns no errors. So there is nothing to check in the ViolationsList

This patch looks great!! Only a couple of tiny nitpicks and it is RTBC from me.

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,119 @@
   +      case static::METADATA_ATTRIBUTE_MIME:
   +        return $file->getMimeType() ?: NULL;
   

   Rather than return NULL if the MIME type is not known, we could inject the MIME type guesser service into this plugin and try to guess the MIME type :) But that's a follow-up, and not an essential one. Just thought I'd mention it!

2. +++ b/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php
   @@ -88,4 +91,43 @@ protected function createMediaType($media_source_name) {
   +   * @param \Drupal\media\Entity\MediaType $media_type
   

   Nit: This should be MediaTypeInterface.

3. +++ b/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php
   @@ -88,4 +91,43 @@ protected function createMediaType($media_source_name) {
   +  protected function generateMedia($filename, MediaType $media_type) {
   

   This should be MediaTypeInterface.

4. +++ b/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php
   @@ -88,4 +91,43 @@ protected function createMediaType($media_source_name) {
   +
   +    vfsStream::setup('drupal_root');
   

   Nit: There's an extra blank line.

Addressing #120

Gorgeous. Pre-RTBC.

And now addressing the comment left from #117 that was not addressed yet. Sorry for overlooking it.

Nice. Back to RTBC on the assumption that the tests will pass.

+++ b/core/modules/media/src/Plugin/media/Source/File.php
@@ -0,0 +1,119 @@
+    ];
+    foreach ($icon_names as $icon_name) {
+      $thumbnail = $icon_base . '/' . $icon_name . '.png';
+      if (is_file($thumbnail)) {
+        return $thumbnail;
+      }
+    }

Does this happen once when the file is uploaded or is it runtime?

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,119 @@
   +    return parent::createSourceField($type)->set('settings', ['file_extensions' => 'txt doc docx pdf']);
   

   This is just defaults right?

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php
   @@ -0,0 +1,75 @@
   +    $page->attachFileToField("files[{$source_field_id}_0]", \Drupal::root() . '/sites/README.txt');
   

   On the one hand this is good re-using the file, on the other the test will break if we change to README.md or something so might be better to have a .txt test file somewhere?

3. +++ b/core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml
   @@ -0,0 +1,44 @@
   +hidden: {  }
   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
   
   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
   new file mode 100644
   
   new file mode 100644
   index 0000000..697f117
   
   index 0000000..697f117
   --- /dev/null
   
   --- /dev/null
   +++ b/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
   +++ b/core/profiles/standard/config/optional/core.entity_view_display.media.file.default.yml
   @@ -0,0 +1,50 @@
   

   Is this really needed in the standard profile?

Responding to #125:

Does this happen once when the file is uploaded or is it runtime?

If I'm grokking things correctly, the answer is "maybe both". getThumbnail() will be called whenever the thumbnail_uri metadata attribute is requested; I think this attribute will be computed and saved when the file is uploaded, but then other code could also call getMetadata('thumbnail_uri') at any time, which would call that function.

This is just defaults right?

Yes. The source field definition is created once, then saved. It's saved as a locked field, but we're working on a related issue to ensure that those configuration values can be changed.

On the one hand this is good re-using the file, on the other the test will break if we change to README.md or something so might be better to have a .txt test file somewhere?

This seems like a bridge we can cross when and if we get to it. I'm speculating here, but it wouldn't hugely surprise me if there are other tests in Drupal that expect README.txt to exist and will break if it gets renamed.

Is this really needed in the standard profile?

Not sure where else it should live. If we put it all in Media, then we are not following standard practice (node types, etc. all ship with the profile). So IMHO it's in the right place...

#125:

On the one hand this is good re-using the file, on the other the test will break if we change to README.md or something so might be better to have a .txt test file somewhere?

I agree we should change this.

Is this really needed in the standard profile?

I agree with #126. If you use something else than Standard profile you are most likely setting these things up yourself anyway.

#125:

2. This seems extremely nitpicky to me…? :)
3. I asked the same question, and again wanted to mark this NW in #85 for that very reason. But then I realized I already asked this before, in #54.36, after which it was partially answered. I raised a follow-up question for it in #74, which was answered by @seanB in #79.

RE: thumbnail calculation happening during upload time or run time: see \Drupal\media\Entity\Media::baseFieldDefinitions(), specifically the thumbnail field. That stores the thumbnail URI. Then, see \Drupal\media\Entity\Media::getThumbnailUri() + \Drupal\media\Entity\Media::updateThumbnail(). That shows the logic: new media items have their thumbnail generated, when the media item is changed, it's refreshed, and otherwise it's just returned from storage. We even have thorough test coverage for this in \Drupal\Tests\media\Kernel\MediaSourceTest::testThumbnail().

Reviewed the code, only found a few things that were not raised above:

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,119 @@
   + *   description = @Translation("Use local files for reusable media."),
   
   +++ b/core/profiles/standard/config/optional/media.type.file.yml
   @@ -0,0 +1,14 @@
   +description: 'Use the "File" media type for storing media locally.'
   

   I think the code doc explanation is much more user friendly and concise. We don't need to repeat it is a media type, also we don't need to repeat the name of it, given the description is displayed with the name, etc.

2. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -0,0 +1,119 @@
   +  public function getMetadataAttributes() {
   +    return [
   +      static::METADATA_ATTRIBUTE_MIME => $this->t('MIME type'),
   +      static::METADATA_ATTRIBUTE_SIZE => $this->t('Size'),
   +    ];
   +  }
   +
   

   These do not have corresponding fields attached to this media bundle. Is that a best practice?

3. +++ b/core/profiles/standard/config/optional/field.field.media.file.field_media_file.yml
   @@ -0,0 +1,29 @@
   +  file_directory: '[date:custom:Y]-[date:custom:m]'
   +  file_extensions: 'txt doc docx pdf'
   

   The lack of editability of these settings was discussed above, #2274433: Do not allow to alter Locked field via UI was added as a related issue, but that deals with the opposite problem. Let's open the followup to discuss this because we lack that issue. @phenaproxima in #126 says "we're working on a related issue to ensure that those configuration values can be changed."

4. +++ b/core/profiles/standard/config/optional/media.type.file.yml
   @@ -0,0 +1,14 @@
   +new_revision: false
   

   Is not supporting new revisions by default intentional or just a matter of accepting defaults as they were? I mean in terms of supporting workflows out of the box, etc.

Tested the user interface as well and found the following problems, some of which may need to be resolved here, some elsewhere:

1. Went to create media. On /admin/content the Media tab is not shown at all. It is shown on /admin/content/comment and /admin/content/files but not on the default tab. I know this is likely a pre-existing condition but this is the first issue that adds a way to add media and people will go and want to actually create media, which they cannot unless they are persistent clicking around tabs that are unrelated. We need an issue for this.
2. The media creation form is a hodge-podge of information. The revision information is above / more important than the file itself. I also did not expect the path alias to be this prominent on the form. Do we have an issue to review the media creation UI?
3. Uploaded this PDF as a test: https://www.drupal.org/files/issues/drupal.poster.1.pdf (bonus history ;), it saved fine but the thumbnail attempts to load /sites/default/files/styles/thumbnail/public/media-icons/generic/generic.png?itok=yWxZ5Pdm which does not work of course, that prints "Error generating image.". And indeed, it looks like an odd combination of an image style path with a hardcoded image path.

Reviewing the issue itself:

1. The issue summary is hugely outdated. It has open unanswered questions, etc. Already has the issue summary update tag.
2. We need a change record to orient people using the media document module and/or extending from the media document module as to what changed, how did the API change, etc. The main media entity patch/module had issues to deal with the migration path, we'll need to consider that here too and open the respective issues and link as related issues, so people can start following/contributing to them immediately, when they see the good news that this lands.

These are all the things I found, overall great job!

1. Agreed. I even suggested that improved label :) Done.
2. That's not the point; the point is that every media source can provide certain metadata attributes. It's then possible (but optional!) to map those media source metadata attributes to media bundle fields. It's the site builder's choice.

   However, it is probably a good idea to have the sole/single/first media source's default configuration actually do this out of the box, because that will be necessary if you want to create a media library that can filter/sort by MIME type and size later on. I defer the specifics to Media initiative experts.
   Great catch! :)

   +++ b/core/profiles/standard/config/optional/media.type.file.yml
   @@ -0,0 +1,14 @@
   +field_map: { }
   

   This is where media source metadata attributes can be mapped to a particular field.

3. +1 that we need a very clear, very well-defined follow-up for this.
4. I defer to the Media initiative experts.

1. On /admin/content the Media tab is not shown at all.

   Yes, I remember there being many problems with this: the root cause is somewhere deep in the routing/menu system. Screenshot of HEAD, without Media:
   

2. I've only been reviewing the code lately, clearly I should've also reviewed the UI. Great catch!
3. Idem dito.

Wonderful review, Gábor — thanks!

Forgot to attach the screenshot.

#129.3:

The lack of editability of these settings was discussed above, #2274433: Do not allow to alter Locked field via UI was added as a related issue, but that deals with the opposite problem. Let's open the followup to discuss this because we lack that issue. @phenaproxima in #126 says "we're working on a related issue to ensure that those configuration values can be changed."

- Updated title of #2274433: Do not allow to alter Locked field via UI to avoid the confusion
- Real follow up of what we want here is at #806102: Locked fields can change the widget but not settings (I guess) not sure if we want to port to D8 or ok to create a separate issue for D8