Problem/Motivation

#2831274: Bring Media entity module to core as Media module has landed Media entities in core, but they aren't of much use without any proper media source plugins.

We need to replicate current file field behavior as per our roadmap (#2825215: Media initiative: Essentials - first round of improvements in core). The first step is parity with file fields, which means supporting local documents/files.

Proposed resolution

Adapt the Document plugin from the Media entity document contributed module, and rename it to File.

Then, create a media source for files (called File), with sane default configuration. This media type will be included with the Standard profile as optional configuration.

Remaining tasks

  1. bring in the File @MediaSource plugin (formerly the Document @MediaType plugin in contrib)
  2. create media type (bundle) for local files, and add it to the Media module
  3. review
  4. commit

Follow-ups

Additionally, #2884202: Non-persistent fields can never be purged without their target entity type was discovered here, but does not need to be fixed by/for the Media initiative. See #188.

Files: 

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

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.

catch’s picture

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

phenaproxima’s picture

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

johndevman’s picture

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

Wim Leers’s picture

#125:

  1. This seems extremely nitpicky to me…? :)
  2. 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().

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

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!

Gábor Hojtsy’s picture

Issue tags: +Needs change record
Wim Leers’s picture

  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!

Wim Leers’s picture

Forgot to attach the screenshot.

vijaycs85’s picture

#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

Gábor Hojtsy’s picture

phenaproxima’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

#129.1: Fixed by @Wim Leers. Thanks!

#129.2 and #131.2: I don't think we need that. Adding fields to store MIME type and size is redundant -- the media type already wraps around a file, which has that information anyway. So we'd effectively be double-storing that information, which means we'd need to keep it in sync if the source field changes, and so forth. It's a can of worms we shouldn't open. Wim raised the point that you might want to create a media library that filters by MIME type -- but you could simply do that via a Views relationship to the wrapped file, rather than mess around with specialized MIME type and size fields.

#129.3: Follow-up confirmed.

#129.4: I see no reason why we should disable revisions out of the box. Let's fix this.

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

Given Wim's comment in #131, it sounds like this is out of scope for us to fix in this issue. Is there an issue already open to address this? /admin/content/media is provided by a view, packaged with Media already. Why isn't it appearing?

Do we have an issue to review the media creation UI?

No. I'll open one.

it looks like an odd combination of an image style path with a hardcoded image path

Uh-oh. Let's fix that here and add test coverage.

Gábor Hojtsy’s picture

I set up two more test sites and neither had the Media tab problem. I had my testing site as a fresh site as well, so don't know why was that different honestly, but it was. But statistically we can ignore that since it worked on the other two attempts on the same patch.

Berdir’s picture

That's likely a known module install/caching issue actually, if you had the local tasks on that page cached before and then install the module without any cache clear then they're not invalidated.

See #2783791: Module install doesn't invalidate render cache

phenaproxima’s picture

Filed #2882801: Review and improve the media creation form to address this point from #129:

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?

phenaproxima’s picture

Adding #2783791: Module install doesn't invalidate render cache as a related issue, as per @Berdir's comment in #137.

phenaproxima’s picture

Adding #2836153: Improve metadata mapping UI on Media type form here, since it came up in IRC discussion today. Specifically, this comment by @seanB:

One usability issue though, people probably wouldn't figure out by themselves that they should create a field to store metadata if they want to, and what type of field they should use.

seanB’s picture

Status: Needs work » Needs review
FileSize
19.39 KB
5.91 KB
  • For #129.2 we discussed removing the metadata fields for the File source. The size and mime type are already added to the file field in the media type. So this would be redundant. Removed the metadata in the new patch.
  • Enabled revisions by default for #129.4
  • Regarding the user interface issue of the thumbnail. It seems that core has a corrupt png of the generic icon in the media module. I added a correct one and it worked again. Even the image styles :). Not sure why the image in core is corrupt though. I fetched the new version from the media entity contrib module and optimized it with ImageOptim.

Follow ups were created for all other issues. Only the CR and related issues for an upgrade path from media_entity_document need to be created.

Wim Leers’s picture

Status: Needs review » Needs work

It seems that core has a corrupt png of the generic icon in the media module. I added a correct one and it worked again.

We've had this problem in the past too. Some git setups seem to corrupt PNG files. Possibly automatic CRLF fixing.

Only the CR and related issues for an upgrade path from media_entity_document need to be created.

Are you saying that sites using https://www.drupal.org/project/media_entity_document should migrate to the File @MediaSource plugin?
The answer is in the IS:

Adapt the Document plugin from the Media entity document contributed module, and rename it to File.

So that was simply the original code. I think I'm hearing we don't want to block this issue on such an upgrade path?


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

    This implementation is now violating the API: it's returning NULL. It must at minimum do return [];.

  2. +++ b/core/profiles/standard/config/optional/media.type.file.yml
    @@ -8,7 +8,7 @@ label: File
    -new_revision: false
    +new_revision: true
    

    Do we need test coverage for this?

Gábor Hojtsy’s picture

I don't think we want to block this on an upgrade/migration solution as we did not do that for the core modules either, but it needs to be mentioned in the change notice and issue opened to work on it eventually so we can prioritize it.

Wim Leers’s picture

as we did not do that for the core modules either

There is an important difference though: this is for a 8.3->8.4 migration. D8 sites that currently exist and use Media will need to update to 8.4 to remain secure. Therefore, their media data needs to migrated too.

We didn't require a 7->8 migration for core modules, but that's different, because those sites can choose to remain on D7 for much longer than the 6 months of support for each Drupal 8 minor.

marcoscano’s picture

D8 sites that currently exist and use Media will need to update to 8.4 to remain secure.

Is there anything in core 8.4.x that would prevent sites from continuing to use the contrib media_entity in its 1.x branch? (I don't think so, but I'm not 100% sure) If that's the case, existing sites could always opt to upgrade drupal without moving to the new "media-in-core" solution (even if this would not be the recommended path).

In practice though, this is something I believe will happen anyways in many cases, because existing sites will be using a bunch of contrib plugin/source modules (not only documents + images), so until all of them have stable 2.x branches compatible with the new API, they will not be able to switch.

phenaproxima’s picture

Therefore, their media data needs to migrated too.

Not exactly :) The data model is essentially unchanged between Media Entity and core Media. The modules just need to upgrade to the new API, and that's it. Media will include an update hook to do the requisite config renames and whatnot, but that's the extent of it.

seanB’s picture

I guess there should be an upgrade path in media_entity_document to move to the file source plugin in core. That's out of scope for this issue, but is definitely be a must have to consider this "stable". Same as the upgrade path for the media_entity module.

Wim Leers’s picture

Well, happy to be wrong! :) But that makes me a bit scared about the upgrade path in general. Where can I read about that? Zero mentions of it in the overarching plan issue: #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core. I think it might be a good idea to document the plan/recommendations that I see being written here in that issue, so that it's A) clear, B) findable.

Gábor Hojtsy’s picture

Yeah all I asked is the upgrade path from the contrib module considered (in this case, the plugin name is not even the same + metadata will be "lost", etc.) and an issue opened at the appropriate place (contrib module or core depending on where it will be implemented) and related to this issue, so people reviewing the change will be aware of the upgrade implications. (Also it does help that *we* discuss the upgrade implications a bit without need to resolve it outright of course).

phenaproxima’s picture

Per @Wim Leers' request in #148, I have documented the plan for the upgrade path over at #2801277-18: [META] Support remote media assets as first-class citizens by adding Media Entity module in core.

Gábor Hojtsy’s picture

Superb. Let's open that issue too for the contrib media document module (or core as appropriate) for upgrade path to this core feature from contrib. Comments in other issues does not work to follow progress on specific tasks for people who need this upgrade path. They also don't work for prioritizing work, etc. :)

phenaproxima’s picture

phenaproxima’s picture

Draft change record written: https://www.drupal.org/node/2883488

seanB’s picture

Status: Needs work » Needs review
FileSize
19.63 KB
2.45 KB
  • Fixed 142.1
  • For 142.2, the new_revision settings is already tested in Drupal\Tests\media\Functional\MediaUiFunctionalTest. This is not something specific for the file source?
  • Also fixed 125.2
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

I think all feedback from Gàbor's review in #129 has been addressed. Let's land this!

andypost’s picture

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

it needs follow-up to add support "odt" extension!

vijaycs85’s picture

@andypost #806102: Locked fields can change the widget but not settings is the follow up trying to allow to edit settings of locked fields.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

Thanks for the links to previous discussion, my concern is a bit different though:

At the moment, media is marked experimental, obviously we're trying to make it stable before 8.4.0 is released (otherwise we'll probably have to remove it from the 8.4.x branch and immediately put it back into 8.5.x to try again).

To do that, we need to keep media self-contained, putting the config into standard (even though it's optional) undermines that self-containment.

So even though that feels like the right place for it to live eventually, either we should open a follow-up to add the configuration, or add it to media for now with a follow-up to move it to standard when media's ready to go stable. That's one extra issue/patch, but worth it to keep things in one place for me.

Discussed this with @xjm and she had a similar opinion.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.36 KB
1.79 KB

+1 to #158 Here it is.

Status: Needs review » Needs work

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

naveenvalecha’s picture

vijaycs85’s picture

looks like the PNG file in the patch changed after I applied locally, although not showing as diff :(

phenaproxima’s picture

Issue summary: View changes

Re #161: Thanks, @naveenvalecha! Added that issue to the roadmap at #2825215-71: Media initiative: Essentials - first round of improvements in core.

phenaproxima’s picture

vijaycs85’s picture

Same patch in #159 with --binary argument when generating the patch (phenaproxima++). diff file in #159 is valid of course.

Status: Needs review » Needs work

The last submitted patch, 165: 2831936-165.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

in #156 I mean to add .odf open document format support because file field defines onlt .txt extension

vijaycs85’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
21.61 KB
4.63 KB

Okay. This should fix the broken tests.

It turns out that the reason they were failing is because they were pretty much written on the assumption that the default configuration would be part of the profile, not the module. Which is a totally reasonable assumption, and one that will be true once we land #2883813: Move File/Image media type into Standard once Media is stable, as we intend to do. Therefore, I decided to take the approach of deleting the default config in setUp(). This might look dubious at first glance, but it clears the way for the tests to pass as written, and will allow them to continue to pass once the config is moved into the profile. All we'll need to do is remove those lines from setUp(), and I added the requisite TODO comments so that we'll know that.

I also had to do a little grooming on the default configuration in order to get DefaultConfigTest to pass, which annoyed the hell out of me, but whatever -- I did what I had to do!

tstoeckler’s picture

+++ b/core/modules/media/config/install/media.type.file.yml
@@ -0,0 +1,12 @@
+  source_field: field_media_file

+++ b/core/modules/media/config/schema/media.schema.yml
@@ -44,6 +44,10 @@ media.source.*:
+media.source.file:
+  type: media.source.field_aware
+  label: '"File" media source configuration'

The source_field configuration value should be added to the schema.

phenaproxima’s picture

Re #170: It already is defined as part of media.source.field_aware, also in media.schema.yml.

tstoeckler’s picture

Oh, wow, that's a really sweet solution! Sorry for the noise and thanks for the quick response.

Status: Needs review » Needs work

The last submitted patch, 169: 2831936-169.patch, failed testing. View results

phenaproxima’s picture

Okay. This should fix the problem with InstallUninstallTest.

It turns out that Media itself did not break InstallUninstallTest, but it did expose a pre-existing condition in the Field module that was manifested under the conditions produced by InstallUninstallTest.

So the changes I've introduced may seem dubious, but believe me...they were necessary in order to get the test to pass. Please see #2884202: Non-persistent fields become unpurgeable zombies without their target entity type for the details of the bug I'm working around in this patch. I'd rather not block this issue on that one, seeing as how I was able to find a workaround and Media is on a deadline. But, I defer to the committers' collective decision.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Nice find phenaproxima. Then I guess we are done here! Back to RTBC.

Wim Leers’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

#150: RE: upgrade path: yay! Updated that issue's IS with your info.

#153: RE: change record: yay! Fixed some nits.

#154: RE: test coverage for new_revision change: if we already have test coverage, then why was #141 able to change this setting, without needing to update any test expectations?

#158: Hah! That makes sense :)

#174:

  1. +++ b/core/modules/media/media.info.yml
    @@ -5,6 +5,5 @@ package: Core (Experimental)
    -  - file
    

    Eh?! Why this change? Won't this make it impossible to use the File media source?

    Or are you doing this because media still depends on image, which itself depends on file, and therefore this is a no-op change in practice? But if that's the case, then why do it?

    (I went through the trouble of manually testing this, so manually uninstalling all the things that caused the file module to be required. Apparently even the update module requires the file module, for no reason AFAICT!)

    This is my only remaining question for now.

  2. +++ b/core/modules/media/media.install
    @@ -28,6 +28,15 @@ function media_install() {
     /**
    + * Implements hook_uninstall().
    + *
    + * @TODO Remove when https://www.drupal.org/node/2884202 is fixed.
    + */
    +function media_uninstall() {
    +  \Drupal::moduleHandler()->invoke('field', 'cron');
    +}
    

    This is weird, but it's to work around a core bug for which there is an issue — this is fine.


I also repeated the manual UI testing that @Gábor Hojtsy did in #129.

  1. The "Media" tab under /admin/content now indeed appears.

    I additionally noticed that the "Media" view is inconsistent with the "Content" and "Files" views:

    1. When no content is available, we don't see the <thead> that those other views do show.
    2. The Media name value is in the middle instead of the very first (leftmost) thing.
    3. The button to apply the exposed filters says Apply instead of Filter
    4. Media: Publishing status, Content: Published status
    5. Provider should be Media source or probably Source

    Generally, we should make the "Media" view as consistent with the "Content" view as possible, since that's the canonical/most used one.

    We should fix this, but that could be done in a follow-up.

  2. RE: media creation form being a hodge-podge, with e.g. revision information above the file: this is still a problem. But we have #2882801: Review and improve the media creation form for that.
  3. RE: uploading PDF and thumbnail being broken. Sadly the image is once again broken… Yes, this is a nightmare. Yes, git is to blame.

And Gábor also had two remarks about the issue itself:

  1. IS outdated. That was fixed in #135. Updated it a bit more.
  2. No CR yet. That now exists. It's rather thin, but AFAICT it covers what we need it to cover.
andypost’s picture

Code looks good except default set of supported extensions

+++ b/core/modules/media/config/install/field.field.media.file.field_media_file.yml
@@ -0,0 +1,29 @@
+  file_extensions: 'txt doc docx pdf'

It needs follow-up to discuss the set to add 'odf' and cover with test this defaults

phenaproxima’s picture

Or are you doing this because media still depends on image, which itself depends on file, and therefore this is a no-op change in practice? But if that's the case, then why do it?

I was trying to figure out what the hell was breaking InstallUninstallTest. I had a theory that the dependency on File was causing it, so I removed it from the info file. It turned out not to be at fault, but I decided to leave the change in place because as you pointed out, Image depends on File, and Media depends on Image (in order to provide the thumbnail base field). So there is no reason for Media to depend explicitly on File.

I will post the requested follow-ups and re-roll the patch with --binary to fix the icon. Can we please, please move to SVG icons (follow-up!) as a matter of course so that we never have to deal with this BS ever again?

phenaproxima’s picture

Adding #2884379: Improve the administrative Media view as a related issue, to address @Wim Leers' points in #176 about the Media view being inconsistent with other administrative content views.

phenaproxima’s picture

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Needs work

#177 + #180: added #2884383 to IS.

#178: But the File @MediaSource plugin does explicitly depend on the file module. So I think it'd be good for media to continue to declare a dependency on file. In any case, it feels out of scope to be changing that in this issue. It's eyebrow-raising, and minimizing committer-eyebrow-raising is always a good thing to do :)

#179: added #2884379 to IS.

RE: #154: test coverage, discussed that during today's Media meeting in IRC. Core committer @xjm agreed with my explanation/justification in #176: that changing that config should have caused some test to fail. This is not about testing the UI change.
I was expecting something along the lines of:

  1. create media
  2. update something in it, such as title
  3. assert that there are 2 revisions

Because it makes a world of difference whether revisioning is enabled or disabled by default for an entity type or bundle! It affects all media entities of that bundle that will be created! Hence the request for this to be tested, so that we can certain that updating them indeed creates new revisions.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
24.55 KB
2.45 KB

Okay. This patch adds the test coverage requested by the final point in #142 and detailed in #181. I also re-added an explicit dependency on File, per #181.

For the record, this patch was rolled with --binary, so the PNG should be intact.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
@@ -0,0 +1,73 @@
+   * Tests a media type that creates new revisions by default.

Not a media type, we want to test the file media type that the default config is creating. i.e. test coverage for the default configuration.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
24.44 KB
1.91 KB

How's this?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
@@ -12,25 +12,21 @@
 class MediaRevisionTest extends MediaFunctionalTestBase {

I think ideally this would be explicitly called MediaDefaultConfigTest or something like that.

To ensure our expectations/assumptions wrt media module's default configuration are actually verifiably true. Of course we don't need to test everything, but this is a good example of something that's worth the burden of test coverage.

Wim Leers’s picture

Clarified things I said in #176 (for which a follow-up was created) with screenshots: #2884379-3: Improve the administrative Media view.

  • catch committed 8c36132 on 8.4.x
    Issue #2831936 by marcoscano, seanB, chr.fritsch, l0ke, Wim Leers,...
catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/modules/media/media.install
@@ -28,6 +28,15 @@ function media_install() {
 
 /**
+ * Implements hook_uninstall().
+ *
+ * @TODO Remove when https://www.drupal.org/node/2884202 is fixed.
+ */
+function media_uninstall() {
+  \Drupal::moduleHandler()->invoke('field', 'cron');
+}
+

I don't mind that as a workaround, field purging is painful and the other issue looks potentially critical.

Committed/pushed to 8.4.x, thanks!

Wim Leers’s picture

Issue summary: View changes

But note that #2884202: Non-persistent fields become unpurgeable zombies without their target entity type is a follow-up that is not really a Media initiative follow-up. It's a bug surfaced by the Media initiative, for which fortunately a decent work-around is available. See #188.

IS updated.

Wim Leers’s picture

I:

  1. postponed #2883813: Move File/Image media type into Standard once Media is stable, because it's blocked on Media becoming stable.
  2. rolled a patch for #2884379: Improve the administrative Media view, it needs review
  3. rolled a patch for #2884383: File media source should accept Open Document format(s), it needs review
  4. assigned #2882801: Review and improve the media creation form and provided steps to provide usability feedback

#806102: Locked fields can change the widget but not settings is apparently also an issue that long predates the Media initiative but might be a blocker for the Media module to become stable.

So, I've done as much "push follow-ups forward" as possible.

phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

Re-tagging.

Status: Fixed » Closed (fixed)

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