Problem/Motivation

When #2831274: Bring Media entity module to core as Media module lands we'll have Media entity in core. But it won't be of much use if we don't provide any proper media plugins.

In order to replicate current file field behavior we need support for local documents/files.

Proposed resolution

Adopt document file plugin from Media entity document contributed module.

Besides that create a media bundle for documents files with sane default configuration. Location of this bundles still needs to be decided as it could be in module that provides media entity, (experimental) module that implements media library (see: #2796001: [prototype] Create design for a Media Library), in standard or in an experimental profile.

Remaining tasks

- Adopt document file media type plugin handler
- decide where media bundles types will be defined
- create media bundle handler for documents

Files: 
CommentFileSizeAuthor
#123 interdiff.txt1.47 KBmarcoscano
#123 2831936-123.patch19.16 KBmarcoscano
#121 interdiff.txt1.08 KBmarcoscano
#121 2831936-121.patch19.11 KBmarcoscano
#116 interdiff-2831936-106-116.txt4.97 KBchr.fritsch
#116 2831936-116.patch19.13 KBchr.fritsch
#106 2831936-diff-102-106.txt3.45 KBvijaycs85
#106 2831936-106.patch18.62 KBvijaycs85
#102 interdiff-2831936-96-102.txt3.02 KBjohndevman
#102 2831936-102.patch18.39 KBjohndevman
#98 media-type-error.png114.77 KBvijaycs85
#98 media-type-locked.png34.29 KBvijaycs85
#98 media-source.png16.73 KBvijaycs85
#96 interdiff-2831936-94-96.txt1.73 KBchr.fritsch
#96 2831936-96.patch16.12 KBchr.fritsch
#94 interdiff-2831936-89-94.txt2.37 KBchr.fritsch
#94 2831936-94.patch16.42 KBchr.fritsch
#90 interdiff-84-89.txt3.46 KBseanB
#90 2831936-89.patch16.53 KBseanB
#84 2831936-84.patch16.99 KBchr.fritsch
#84 interdiff-2831936-83-84.txt748 byteschr.fritsch
#83 2831936-83.patch16.98 KBchr.fritsch
#80 iterdiff-77-80.txt1.43 KBl0ke
#80 2831936-80-plugin-only-do-not-test.patch17.09 KBl0ke
#80 2831936-80-full.patch263.54 KBl0ke
#77 interdiff-73-77.txt12.99 KBl0ke
#77 2831936-77-plugin-only-do-not-test.patch16.89 KBl0ke
#77 2831936-77-full.patch263.34 KBl0ke
#73 interdiff-69-73.txt2.97 KBseanB
#73 2831936-73-plugin-only-do-not-test.patch16.01 KBseanB
#73 2831936-73-full.patch260.26 KBseanB
#69 interdiff-67-69.txt9.54 KBseanB
#69 2831936-69-plugin-only-do-not-test.patch15.37 KBseanB
#69 2831936-69-full.patch259.61 KBseanB
#67 interdiff-64-67.txt17.52 KBseanB
#67 2831936-67-plugin-only-do-not-test.patch15.17 KBseanB
#67 2831936-67-full.patch259.41 KBseanB
#65 media_file_plugin-2831936-64.patch14.98 KBdrubb
#65 interdiff-2831936-62-64.txt2.03 KBdrubb
#62 media_file_plugin-2831936-62.patch15.02 KBWim Leers
#4 2831936-4-do-not-test.patch4.01 KBmarcoscano
#5 2831936-5-do-not-test.patch4.74 KBmarcoscano
#8 2831936-8-full.patch181.68 KBmarcoscano
#8 2831936-8-only-plugin-do-not-test.patch12.41 KBmarcoscano
#10 2831936-8-full.patch239.31 KBmarcoscano
#11 2831936-11-only-plugin-do-not-test.patch12.67 KBmarcoscano
#13 2831936-13-full.patch239.3 KBmarcoscano
#13 2831936-13-only-plugin-do-not-test.patch12.66 KBmarcoscano
#17 interdiff.txt6.74 KBmarcoscano
#17 2831936-17-full.patch239.78 KBmarcoscano
#17 2831936-17-only-plugin-do-not-test.patch13.14 KBmarcoscano
#19 interdiff.txt713 bytesmarcoscano
#19 2831936-19-full.patch239.79 KBmarcoscano
#19 2831936-19-only-plugin-do-not-test.patch13.15 KBmarcoscano
#21 interdiff.txt611 bytesmarcoscano
#21 2831936-21-full.patch239.76 KBmarcoscano
#21 2831936-21-only-plugin-do-not-test.patch13.13 KBmarcoscano
#23 2831936-23-only-plugin-do-not-test.patch12.19 KBmarcoscano
#24 interdiff.txt7.75 KBmarcoscano
#24 2831936-24-only-plugin-do-not-test.patch10.48 KBmarcoscano
#25 2831936-25-full.patch199 KBmarcoscano
#25 2831936-25-only-plugin-do-not-test.patch10.74 KBmarcoscano
#25 interdiff.txt969 bytesmarcoscano
#29 2831936-29-full.patch190.75 KBpguillard
#29 2831936-29-only-plugin-do-not-test.patch10.51 KBpguillard
#31 2831936-31-plugin-only-do-not-test.patch12.48 KBmarcoscano
#31 2831936-31-full.patch193.05 KBmarcoscano
#33 interdiff.txt1.43 KBmarcoscano
#33 2831936-33-plugin-only-do-not-test.patch12.46 KBmarcoscano
#33 2831936-33-full.patch193.02 KBmarcoscano
#36 interdiff.txt4.1 KBmarcoscano
#36 2831936-36-plugin-only-do-not-test.patch12.07 KBmarcoscano
#36 2831936-36-full.patch195.3 KBmarcoscano
#37 interdiff.txt1.97 KBmarcoscano
#37 2831936-37-plugin-only-do-not-test.patch11.97 KBmarcoscano
#37 2831936-37-full.patch195.46 KBmarcoscano
#40 2831936-40-full.patch192.3 KBchr.fritsch
#40 2831936-40-plugin-only-do-not-test.patch14.05 KBchr.fritsch
#40 interdiff-2831936-37-40.txt4.38 KBchr.fritsch
#43 2831936-43-full.patch199.33 KBseanB
#43 2831936-43-plugin-only-do-not-test.patch14.18 KBseanB
#43 interdiff_40_43.txt1.99 KBseanB
#45 2831936-45-full.patch200.9 KBseanB
#45 2831936-45-plugin-only-do-not-test.patch15.76 KBseanB
#45 interdiff_43_45.txt5.18 KBseanB
#47 2831936-47-full.patch200.03 KBseanB
#47 2831936-47-plugin-only-do-not-test.patch14.89 KBseanB
#47 interdiff_45_47.txt1.54 KBseanB
#55 Screen Shot 2017-01-17 at 13.47.19.png18.67 KBWim Leers

Comments

slashrsm created an issue. See original summary.

marcoscano’s picture

Issue summary: View changes

Fixed typo from copy/pasting

marcoscano’s picture

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: [PP-1] Add "Image" MediaSource plugin

marcoscano’s picture

FileSize
4.01 KB

untested WIP

marcoscano’s picture

Status: Active » Needs review
FileSize
4.74 KB
marcoscano’s picture

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?

marcoscano’s picture

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)

marcoscano’s picture

The last submitted patch, 8: 2831936-8-full.patch, failed testing.

marcoscano’s picture

now with full index line in the patch

marcoscano’s picture

The last submitted patch, 10: 2831936-8-full.patch, failed testing.

marcoscano’s picture

phenaproxima’s picture

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.

The last submitted patch, 13: 2831936-13-full.patch, failed testing.

chr.fritsch’s picture

Status: Needs review » Needs work
marcoscano’s picture

Status: Needs work » Needs review
FileSize
6.74 KB
239.78 KB
13.14 KB

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

The last submitted patch, 17: 2831936-17-full.patch, failed testing.

marcoscano’s picture

This should deal with the testbot complain

The last submitted patch, 19: 2831936-19-full.patch, failed testing.

marcoscano’s picture

testbot I still love you

The last submitted patch, 19: 2831936-19-full.patch, failed testing.

marcoscano’s picture

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.

marcoscano’s picture

Title: Add "Document" media type plugin » Add "File" media type plugin
Issue summary: View changes
FileSize
7.75 KB
10.48 KB

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.

marcoscano’s picture

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

dawehner’s picture

  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

The last submitted patch, 25: 2831936-25-full.patch, failed testing.

pguillard’s picture

Assigned: Unassigned » pguillard
pguillard’s picture

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

Status: Needs review » Needs work

The last submitted patch, 29: 2831936-29-only-plugin-do-not-test.patch, failed testing.

marcoscano’s picture

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.

marcoscano’s picture

Status: Needs work » Needs review

testbot please do your work

marcoscano’s picture

Component: entity system » media system
FileSize
1.43 KB
12.46 KB
193.02 KB

Missed the renaming in two spots, corrected now.

The last submitted patch, 31: 2831936-31-full.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2831936-33-full.patch, failed testing.

marcoscano’s picture

Assigned: pguillard » Unassigned
Status: Needs work » Needs review
FileSize
4.1 KB
12.07 KB
195.3 KB

Rebased after 2831274-120, and refactored accordingly.

marcoscano’s picture

Rebased and refactored on top of 2831274-136

Status: Needs review » Needs work

The last submitted patch, 37: 2831936-37-full.patch, failed testing.

mtodor’s picture

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.

chr.fritsch’s picture

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

The last submitted patch, 40: 2831936-40-full.patch, failed testing.

phenaproxima’s picture

Status: Needs review » Needs work
  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.

seanB’s picture

Status: Needs work » Needs review
FileSize
199.33 KB
14.18 KB
1.99 KB

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.

The last submitted patch, 43: 2831936-43-full.patch, failed testing.

seanB’s picture

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.

The last submitted patch, 45: 2831936-45-full.patch, failed testing.

seanB’s picture

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

tedbow’s picture

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.

seanB’s picture

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 :)

slashrsm’s picture

Title: Add "File" media type plugin » Add "File" media handler plugin

Icons logic is still more or less the same as it is in contrib. Note that there is #2833768: Add media type icon SVG files for all the media types to core icons, which definitely influences this. I expect icon logic to change/adapt based on what is done there.

Gábor Hojtsy’s picture

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?

tedbow’s picture

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.

tedbow’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

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:

Wim Leers’s picture

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.

Wim Leers’s picture

Wim Leers’s picture

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

naveenvalecha’s picture

Title: Add "File" media handler plugin » [PP-1] Add "File" media handler plugin
Status: Needs work » Postponed

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

Wim Leers’s picture

Wim Leers’s picture

FileSize
14.92 KB
1014 bytes
Wim Leers’s picture

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drubb’s picture

Keeping up with API changes at

drubb’s picture

seanB’s picture

Assigned: Unassigned » seanB
seanB’s picture

Status: Postponed » Needs review
FileSize
259.41 KB
15.17 KB
17.52 KB

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!

seanB’s picture

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?

seanB’s picture

seanB’s picture

Sorry, 54.2 is also still TODO!

marcoscano’s picture

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.

mtodor’s picture

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"

seanB’s picture

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

Wim Leers’s picture

Title: [PP-1] Add "File" media handler plugin » [PP-1] Add "File" MediaSource plugin

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:


#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.'
    
l0ke’s picture

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.

The last submitted patch, 75: 2831936-75-full.patch, failed testing.

l0ke’s picture

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

The last submitted patch, 77: 2831936-77-full.patch, failed testing.

seanB’s picture

Status: Needs review » Needs work

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.

l0ke’s picture

Thank you @seanB!

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

phenaproxima’s picture

phenaproxima’s picture

Issue tags: +Needs reroll

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

chr.fritsch’s picture

Assigned: seanB » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.98 KB

Here is the reroll

chr.fritsch’s picture

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?

Wim Leers’s picture

Title: [PP-1] Add "File" MediaSource plugin » Add "File" MediaSource plugin
Status: Needs review » Reviewed & tested by the community

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.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

Wim Leers’s picture

#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.
seanB’s picture

Status: Needs work » Needs review
FileSize
16.53 KB
3.46 KB

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

The last submitted patch, 80: 2831936-80-full.patch, failed testing.

phenaproxima’s picture

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.

phenaproxima’s picture

Status: Needs review » Needs work
  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...?

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
16.42 KB
2.37 KB

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.

phenaproxima’s picture

Status: Needs review » Needs work

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. :)

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
16.12 KB
1.73 KB

Ok, adjusted comments 4 and 5 form #93

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

My feedback, she is addressed. Restoring RTBC!

vijaycs85’s picture

Issue summary: View changes
FileSize
16.73 KB
34.29 KB
114.77 KB

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?

phenaproxima’s picture

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

vijaycs85’s picture

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.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

I agree...but let's not support image extensions in this issue. We'll do that in #2831937: [PP-1] 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).

johndevman’s picture

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!

johndevman’s picture

Status: Needs work » Needs review
phenaproxima’s picture

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!

phenaproxima’s picture

Status: Needs review » Needs work

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. :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
3.45 KB

Fixed all items in #105

naveenvalecha’s picture

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"

Status: Needs review » Needs work

The last submitted patch, 106: 2831936-106.patch, failed testing.

seanB’s picture

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.

Wim Leers’s picture

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

johndevman’s picture

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

seanB’s picture

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

Wim Leers’s picture

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.

seanB’s picture

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?

chr.fritsch’s picture

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.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
19.13 KB
4.97 KB

Fixed all the comments from #107

naveenvalecha’s picture

+++ 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: Locked field can be altered 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

vijaycs85’s picture

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?

chr.fritsch’s picture

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

phenaproxima’s picture

Status: Needs review » Needs work

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.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
19.11 KB
1.08 KB

Addressing #120

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Gorgeous. Pre-RTBC.

marcoscano’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
19.16 KB
1.47 KB

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

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

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