Problem/Motivation

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

In order to replicate current image field behavior we need support for local images.

Proposed resolution

Adopt image plugin from Media entity image contributed module.

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

Remaining tasks

- Adopt image media type plugin
- decide where media bundles will be defined
- create media bundle for local images

Files: 
CommentFileSizeAuthor
#50 interdiff-48-50.txt5.17 KBseanB
#50 2831937-50-plugin-only-do-not-test.patch35.95 KBseanB
#50 2831937-50-full.patch51.7 KBseanB
#48 iterdiff-46-48.txt756 bytesl0ke
#48 2831937-48-plugin-only-do-not-test.patch38.43 KBl0ke
#48 2831937-48-full.patch301.02 KBl0ke
#46 iterdiff-44-46.txt20.45 KBl0ke
#46 2831937-46-plugin-only-do-not-test.patch38.43 KBl0ke
#46 2831937-46-full.patch301.02 KBl0ke
#44 interdiff-42-44.txt3.51 KBseanB
#44 2831937-44-plugin-only-do-not-test.patch25.96 KBseanB
#44 2831937-44-full.patch285.73 KBseanB
#42 interdiff-41-42.txt5.4 KBseanB
#42 2831937-42.patch26.6 KBseanB
#41 2831937-41.patch26.36 KBwebflo
#41 2831937-41.interdiff.txt4.41 KBwebflo
#39 media_image_plugin-2831937-39.patch26.5 KBwebflo
#35 media_image_plugin-2831937-35.patch26.46 KBWim Leers
#26 thumbnail_not_updating.png286.74 KBtedbow
#24 interdiff_23_24.txt4.36 KBseanB
#24 2831937-24-plugin-only-do-not-test.patch26.5 KBseanB
#24 2831937-24-full.patch226.05 KBseanB
#23 2831937_23-interdiff-23-22.txt3.25 KBmtodor
#23 2831937_23-do-not-test.patch29.83 KBmtodor
#22 2831937_interdiff-22-19.txt8.68 KBmtodor
#22 2831937_22-do-not-test.patch28.46 KBmtodor
#19 2831937_19-full.patch212.73 KBmtodor
#19 2831937_19-do-not-test.patch22.77 KBmtodor
#17 2831937_17-full.patch219.03 KBmtodor
#14 2831937_14-interdiff-14-13.txt11.45 KBmtodor
#14 2831937_14-do-not-test.patch25.34 KBmtodor
#13 2831937_13-interdiff-13-12.txt12.37 KBmtodor
#13 2831937_13-do-not-test.patch23.44 KBmtodor
#12 2831937_12-interdiff-12-6.txt10.67 KBmtodor
#12 2831937_12-for-testing.patch195.51 KBmtodor
#12 2831937_12-do-not-test.patch13.93 KBmtodor
#5 2831937_5.patch11.4 KBmtodor
#5 2831937_5_for_testing.patch180.38 KBmtodor
#3 2831937_3_no_test.patch11.04 KBmtodor
#3 2831937_3_for_testing.patch180.03 KBmtodor

Comments

slashrsm created an issue. See original summary.

mtodor’s picture

Assigned: Unassigned » mtodor
mtodor’s picture

Status: Active » Needs review
FileSize
180.03 KB
11.04 KB

Here is initial migration of Entity Media Image from "media_entity_image" module into core "media_entity" module.

As code base patch #61 from [#] is used. Relevant changes for Image Media are in "no_test" patch.

Remaining tasks

  1. Add tests from Media Image
mtodor’s picture

Status: Needs review » Needs work
mtodor’s picture

Reading of EXIF data didn't work, it should be fixed with this patch.

mtodor’s picture

Status: Needs work » Needs review

Rolling out tests.

slashrsm’s picture

  1. +++ b/core/modules/media_entity/config/schema/media_entity.schema.yml
    @@ -36,6 +36,17 @@ media_entity.bundle.*:
    +  label: 'Media Image type configuration'
    

    What used to be types are not handlers.

  2. +++ b/core/modules/media_entity/config/schema/media_entity.schema.yml
    @@ -36,6 +36,17 @@ media_entity.bundle.*:
    +      label: 'Field with embed code/URL'
    

    This is image field. I assume this was wrong originally :)

  3. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Image.php
    @@ -0,0 +1,296 @@
    +    $fields = array(
    ...
    +      $fields += array(
    

    Use short array syntax consistently throughout the file.

  4. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Image.php
    @@ -0,0 +1,296 @@
    +    $form['gather_exif'] = [
    +      '#type' => 'select',
    +      '#title' => $this->t('Whether to gather EXIF data.'),
    +      '#description' => $this->t('Gather EXIF data, if that functionality is provided.'),
    +      '#default_value' => empty($this->configuration['gather_exif']) || $this->canReadExifData() ? $this->configuration['gather_exif'] : 0,
    +      '#options' => [
    +        0 => $this->t('No'),
    +        1 => $this->t('Yes'),
    +      ],
    

    UX team would prefer this to be checkbox.

    https://docs.google.com/document/d/1BnkA4fXZWG1nC4ipyl90BSvnE6Buqyjuz4gT...

    This was originally added mostly to be able to disable it if exif support is not available in PHP. Maybe we can always enable that and just display a message if we detect that support is missing?

  5. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Image.php
    @@ -0,0 +1,296 @@
    +        'callback' => '::ajaxTypeProviderData',
    

    This will change to "::ajaxHandlerData" in the upstream patch.

Gábor Hojtsy’s picture

This was originally added mostly to be able to disable it if exif support is not available in PHP. Maybe we can always enable that and just display a message if we detect that support is missing?

A yes/no thing should normally be a checkbox. If its not really a user choice then we should not display it. I *thought* it may be that taking the EXIF data takes time/resources and this may be a performance setting? But we don't have that kind of setting for the rest of the metadata which may also take considerable time to produce.

slashrsm’s picture

I *thought* it may be that taking the EXIF data takes time/resources and this may be a performance setting?

There is some performance impact obviously, but data won't be loaded until needed.

Gábor Hojtsy’s picture

There is some performance impact obviously, but data won't be loaded until needed.

All right, then automating this would be the best from a UX perspective for sure.

dawehner’s picture

  1. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Image.php
    @@ -0,0 +1,296 @@
    +    $file = $media->{$source_field}->entity;
    ...
    +    if ($file = $media->{$source_field}->entity) {
    

    Personally I just prefer to use $media->get($source_field) here, it provides more information and is less magic and we don't need the {} weirdness

  2. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Image.php
    @@ -0,0 +1,296 @@
    +        in_array($field->getType(), $allowed_field_types)
    ...
    +    if (!empty($source_field) && ($file = $media->{$source_field}->entity)) {
    

    We should use strict checking, see https://3v4l.org/9k0Bt

  3. +++ b/core/modules/media_entity/src/Plugin/MediaEntity/Type/Image.php
    @@ -0,0 +1,296 @@
    +    $form['gather_exif'] = [
    +      '#type' => 'select',
    +      '#title' => $this->t('Whether to gather EXIF data.'),
    +      '#description' => $this->t('Gather EXIF data, if that functionality is provided.'),
    +      '#default_value' => empty($this->configuration['gather_exif']) || $this->canReadExifData() ? $this->configuration['gather_exif'] : 0,
    +      '#options' => [
    +        0 => $this->t('No'),
    +        1 => $this->t('Yes'),
    +      ],
    +      '#ajax' => [
    +        'callback' => '::ajaxTypeProviderData',
    +      ],
    +      '#disabled' => !$this->canReadExifData(),
    +    ];
    

    Another reason IMHO to not have the setting in the first place is that you already do an explicit action by setting up a field mapping for these additional fields anyway, so you kinda opt into additional behaviour already.

mtodor’s picture

Here are patches with comments from #7 and #11.
Relevant patch is: 2831937_12-do-not-test.patch -> changes are based on patch #61 from #2831274: Bring Media entity module to core as Media module. And also functionality from #2831936: Add "File" MediaSource plugin.

For comment #7:

  1. adjusted
  2. adjusted
  3. fixed
  4. this is removed, based on #10
  5. it's related to point 4.) - so it's also removed

For comment #11:

  1. this is moved to separate method and get() is used now
  2. adjusted
  3. this is removed, based on #10

Base test is added, but it's not final. It should be enhanced, to test few additional things:

  1. test fetching of file data
  2. test fetching of image data
  3. test fetching of EXIF data
  4. test creation of thumbnail for image
mtodor’s picture

Status: Needs review » Needs work
FileSize
23.44 KB
12.37 KB

Here are added tests listed in #12.

Fixture JPEG image with EXIF data is added, so that we can check reading of EXIF data.

mtodor’s picture

Rebase on top of #25 of #2831936: Add "File" MediaSource plugin.

Also default source field is provided.

mtodor’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2831937_14-full.patch, failed testing.

mtodor’s picture

Status: Needs work » Needs review
FileSize
219.03 KB

Fixed Full patch for testing. Other patches - do-not-test and interdiff are correct.

Status: Needs review » Needs work

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

mtodor’s picture

Status: Needs work » Needs review
FileSize
22.77 KB
212.73 KB

Here is rebase on top of #37 -> #2831936-37: Add "File" MediaSource plugin.

Status: Needs review » Needs work

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

slashrsm’s picture

  1. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,242 @@
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    +    return new static(
    +      $configuration,
    +      $plugin_id,
    +      $plugin_definition,
    +      $container->get('entity_type.manager'),
    +      $container->get('entity_field.manager'),
    +      $container->get('config.factory'),
    +      $container->get('image.factory'),
    +      $container->get('file_system')
    +    );
    +  }
    

    I added another dependency in https://www.drupal.org/node/2831274#comment-11827943. Sorry :(

  2. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,242 @@
    +    $fields += [
    +      'width' => $this->t('Width'),
    +      'height' => $this->t('Height'),
    +    ];
    ...
    +    if ($this->canReadExifData()) {
    +      $fields += [
    +        'model' => $this->t('Camera model'),
    +        'created' => $this->t('Image creation datetime'),
    +        'iso' => $this->t('Iso'),
    +        'exposure' => $this->t('Exposure time'),
    +        'aperture' => $this->t('Aperture value'),
    +        'focal_length' => $this->t('Focal length'),
    +      ];
    +    }
    

    This values now need to be an array with current strings under "label" key.

mtodor’s picture

Added default configurations and adjusted comments written in #21.

mtodor’s picture

Adjustments related to #159 from #2831274: Bring Media entity module to core as Media module - for full stack testing patch, adjustments to mentioned patch are required in #2831936: Add "File" MediaSource plugin.

seanB’s picture

Rebased on #2831936-47: Add "File" MediaSource plugin. Renamed the getSourceFieldValue function to getSourceFile. Also removed some generic test functions now added in MediaHandlerTestBase and moved the changes for MediaHandlerTestBase to the patch in #2831936-47: Add "File" MediaSource plugin.

seanB’s picture

Status: Needs work » Needs review

Should be ready to test...

tedbow’s picture

FileSize
286.74 KB

I am getting weird behaviour where the thumbnail of the image remains of the first image that uploaded even if I remove the image and upload another 1.
In this screenshot I am displaying both the thumbnail field and the image field itself. You can see they don't match. I uploaded the drop image first.

tedbow’s picture

Status: Needs review » Needs work

When installing the media module with this patch it creates the Image media type but the label of the source field is the same as the machine name "field_media_image". The File media type on other hand has label "File" and machine name "field_media_file". I am not sure why.

tedbow’s picture

+++ b/core/profiles/standard/config/optional/field.field.media.image.field_media_image.yml
@@ -0,0 +1,40 @@
+label: field_media_image

This was the problem with the field label mentioned in #27. I was only looking under core/modules/media not the profile.

seanB’s picture

I think #26 is an issue in the main media patch. I will add a solution there and provide an updated patch for this issue when it's approved.
Changing the field label to be in line with the File type sounds logical. I will fix that as well.

slashrsm’s picture

Title: Add "Image" media type plugin » Add "Image" media handler plugin
Wim Leers’s picture

@mtodor: first of all, AWESOME AVATAR! Totoro FTW!

Overall, all remarks in #2831936-54: Add "File" MediaSource plugin should also be cross-checked against this patch. I see a lot of the same problems.

  1. +++ b/core/modules/media/config/schema/media.schema.yml
    --- /dev/null
    +++ b/core/modules/media/images/icons/no-thumbnail.png
    

    Image needs to be optimized.

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

    Same remark as in #2831936-54: Add "File" MediaSource plugin.

  3. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    + *   allowed_field_types = {"image", "file"}
    

    Huh?

  4. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +      'width' => ['label' => $this->t('Width')],
    +      'height' => ['label' => $this->t('Height')],
    ...
    +        'model' => ['label' => $this->t('Camera model')],
    

    Same remark as in #2831936-54: Add "File" MediaSource plugin.

  5. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +        'iso' => ['label' => $this->t('Iso')],
    

    s/Iso/ISO/

  6. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +      ->get('icon_base') . '/no-thumbnail.png';
    

    Same remark as in #2831936-54: Add "File" MediaSource plugin.

  7. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +   * Check does functionality for reading EXIF data exist.
    

    Needs rewording.

  8. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +   * Get EXIF field value.
    

    s/Get/Gets/

  9. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +   * Read EXIF.
    

    s/Read/Reads/

  10. +++ b/core/modules/media/src/Plugin/media/Handler/Image.php
    @@ -0,0 +1,246 @@
    +   *   The uri for the file that we are getting the Exif.
    

    s/uri/URI/
    s/Exif/EXIF/
    Also, broken English.

  11. +++ b/core/modules/media/tests/fixtures/exif_example.jpeg
    @@ -0,0 +1,50 @@
    +  <exif:Model>Drupal EXIF Camera</exif:Model>
    +  <exif:XResolution>72</exif:XResolution>
    +  <exif:YResolution>72</exif:YResolution>
    +  <exif:ResolutionUnit>Inch</exif:ResolutionUnit>
    +  <exif:YCbCrPositioning>Centered</exif:YCbCrPositioning>
    +  <exif:ExifVersion>Exif Version 2.1</exif:ExifVersion>
    +  <exif:FlashPixVersion>FlashPix Version 1.0</exif:FlashPixVersion>
    +  <exif:ColorSpace>Uncalibrated</exif:ColorSpace>
    +  <exif:InteroperabilityIndex>N</exif:InteroperabilityIndex>
    +  <exif:InteroperabilityVersion>52, 30, 32.751</exif:InteroperabilityVersion>
    +  <exif:GPSLongitudeRef>E</exif:GPSLongitudeRef>
    +  <exif:GPSLongitude>13, 22, 25.5432</exif:GPSLongitude>
    

    Hah, so this is what EXIF data looks like. The return of RDF! :)

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

    Same remark as in #2831936-54: Add "File" MediaSource plugin.

  13. +++ b/core/modules/media/tests/src/FunctionalJavascript/ImageTest.php
    @@ -0,0 +1,79 @@
    +    // Create a supported and a non-supported field.
    +    $fields = [
    +      'field_string_mime' => 'string',
    +      'field_string_width' => 'string',
    +      'field_string_model' => 'string',
    +    ];
    

    Same remark as in #2831936-54: Add "File" MediaSource plugin.

  14. +++ b/core/profiles/standard/config/optional/field.field.media.image.field_media_image.yml
    --- /dev/null
    +++ b/core/profiles/standard/config/optional/field.storage.media.field_media_image.yml
    

    Same remark as in #2831936-54: Add "File" MediaSource plugin.

naveenvalecha’s picture

Title: Add "Image" media handler plugin » [PP-1] Add "Image" media handler plugin
Component: entity system » media system
Status: Needs work » Postponed

Thank you everyone.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

Also, the IS doesn't state what this enables that #2831936: Add "File" MediaSource plugin doesn't already bring.

Wim Leers’s picture

Wim Leers’s picture

mtodor’s picture

Assigned: mtodor » Unassigned

I forgot to un-assign long time ago. :/

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.

mondrake’s picture

#31.11

Hah, so this is what EXIF data looks like. The return of RDF! :)

Not really. This is probably a dump in almost human-readable format added by an image editor to some data segment within the image file.

EXIF data is stored in a different format, as 'tags' into 'ifds' segments of the JPEG file format.

Esoteric read :) in https://www.media.mit.edu/pia/Research/deepview/exif.html

webflo’s picture

seanB’s picture

  1. +++ b/core/modules/media/config/schema/media.schema.yml
    @@ -40,6 +40,10 @@ media.source.file:
       label: 'File handler configuration'
    

    \source

  2. +++ b/core/modules/media/config/schema/media.schema.yml
    @@ -40,6 +40,10 @@ media.source.file:
    +media.handler.image:
    +  type: media.handler.field_aware
    +  label: 'Image handler configuration'
    

    It should say 'source' in stead of 'handler'.

  3. +++ b/core/modules/media/images/icons/no-thumbnail.png
    @@ -0,0 +1,6 @@
    IHDR�����etEXtSoftwareAdobe ImageReadyq�e<0IDATx���1O"[��ݛb6TRQYA��4Ra���쬬��
    

    We should probably optimize the image to remove this Adobe stuff.

  4. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,246 @@
    +    $file = $media->get($this->configuration['source_field'])->entity;
    

    I'm not 100% sure if the source field could be empty? But maybe we should check just in case.

  5. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,246 @@
    +        return $file->getFileUri();
    

    Use $uri since we got it anyway.

  6. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,246 @@
    +  public function getDefaultThumbnail() {
    +    return $this->configFactory->get('media.settings')
    +      ->get('icon_base') . '/no-thumbnail.png';
    +  }
    

    This is now a annotation default_thumbnail_filename.

  7. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
    @@ -0,0 +1,79 @@
    +    $this->drupalGet("admin/structure/media/manage/$type_name");
    

    Use {$type_name}

  8. +++ b/core/profiles/standard/config/optional/field.storage.media.field_media_image.yml
    @@ -0,0 +1,32 @@
    +locked: false
    

    This should be true.

  9. +++ b/core/profiles/standard/config/optional/media.type.image.yml
    @@ -0,0 +1,14 @@
    +handler: image
    ...
    +handler_configuration:
    

    We need to change 'handler' to 'source.'

webflo’s picture

Fixed everything from #40

seanB’s picture

Status: Postponed » Needs work
FileSize
26.6 KB
5.4 KB

So I just addressed some of the comments from #31 that were not done yet. Also kept up with some of the changes in #69 of #2831936: Add "File" MediaSource plugin. Tests are still broken but we can fix it later. Uploading for now.

Still todo:
#31.1
#31.14

Done:
#31.1 TODO. The image got messed up somewhere. Maybe we have a good one in a older patch.
#31.2 Done.
#31.3 Done
#31.4 Done
#31.5 Done
#31.6 Done
#31.7 Done
#31.8 Done
#31.9 Done
#31.10 Done
#31.11 LOL
#31.12 Done
#31.13 Done
#31.14 TODO

mtodor’s picture

Very nice! I did also quick check here as for file source plugin #2831936: Add "File" MediaSource plugin.

Here are few things I have noticed:

  1. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,241 @@
    +
    +    if ($value !== NULL) {
    +      return $value;
    +    }
    

    If I'm not wrong, this will pick-up things that we don't want (fe. 'thumbnail_uri'). I think custom data fetching from child should be executed first, before fallback onto parent. So this should be moved to end, instead of return NULL;, just return parent::getMetadata($media, $name);. I guess.

  2. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,241 @@
    +        return $image->getWidth() ?: FALSE;
    ...
    +        return $image->getHeight() ?: FALSE;
    

    This should be ?: NULL; since NULL is now "no value". I think same changes should be done for #2831936: Add "File" MediaSource plugin.

  3. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,241 @@
    +    return $this->exif[$field] ?: FALSE;
    

    This should also be ?: NULL; since NULL is now "no value". And comment should be adjusted accordingly.

seanB’s picture

Status: Needs work » Needs review
FileSize
285.73 KB
25.96 KB
3.51 KB

#31.1 Done
#31.14 We discussed this today. Since we don't want to always install the module config we decided the standard profile was the best location.

#43.1 Done
#43.2 Done
#43.3 Done

Wim Leers’s picture

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

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

  • #31.4 (constants)
  • #31.10 (broken English): partially fixed. Suggested fix: The URI for the file that we are getting the EXIF for.

#38: hah! Thanks for that :)


And finally, a complete review:

  1. +++ b/core/modules/media/config/schema/media.schema.yml
    @@ -40,6 +40,10 @@ media.source.file:
    +  label: 'Image source configuration'
    

    For consistency with #2831936: Add "File" MediaSource plugin:

    '"Image" media source configuration'
    
  2. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    + *   description = @Translation("Allows local images to be used as media."),
    

    Same remark as at #2831936-74: Add "File" MediaSource plugin.2. I suggest

    Use local images for reusable media.

  3. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +  /**
    +   * The EXIF data.
    +   *
    +   * @var array
    +   */
    +  protected $exif;
    

    So… is this a static cache? This is confusing.

    EDIT: looking at getExifField(), it indeed is a static cache, for the file being currently read.

    We need test coverage to prove that when multiple files are read in succession, that it doesn't reuse the statically cached EXIF data for the current file.

  4. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +    if (!$file) {
    +      return NULL;
    +    }
    

    When can this happen? (Also asked at #2831936-74: Add "File" MediaSource plugin.5.)

  5. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +    // Return the field.
    

    This comment is useless, and wrong! Let's delete it.

  6. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +    // Show message if it's not possible to read EXIF data.
    +    if (!$this->canReadExifData()) {
    

    Pointless comment. Let's delete it.

  7. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +        '#markup' => $this->t('Unable to read EXIF data for Image. In order to provide EXIF data reading functionality please take a look at PHP documentation of <a href="https://secure.php.net/exif_read_data" target="_blank">exif_read_data</a> function.'),
    

    s/Image/images/
    s/of [function]/of the [function]/

  8. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +   *   The uri for the file that we are getting the EXIF.
    

    s/uri/URI/
    s/the EXIF/the EXIF field value/

  9. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +   *   The name of the EXIF field.
    

    s/the EXIF field/the requested EXIF field/

  10. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +   *   The value for the requested field or FALSE if is not set.
    

    s/if is/if it is/

  11. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,235 @@
    +   *   An associative array where the indexes are header names and values are
    

    s/indexes/keys/

  12. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
    @@ -0,0 +1,82 @@
    +    // Create image media source.
    +    $this->createMediaTypeTest($type_id, 'image', $provided_fields);
    

    Same remark as #2831936-74: Add "File" MediaSource plugin.7.

  13. +++ b/core/profiles/standard/config/optional/field.field.media.image.field_media_image.yml
    @@ -0,0 +1,40 @@
    +  file_extensions: 'png gif jpg jpeg'
    +  alt_field: true
    +  alt_field_required: true
    +  title_field: false
    +  title_field_required: false
    +  max_resolution: ''
    +  min_resolution: ''
    

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

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

    Just like #2831936-74: Add "File" MediaSource plugin.19: "uploading local images" sounds fairly strange.

    What about:

    'Use the "Image" media type for storing image media locally.'
    
l0ke’s picture

#45 Done.
#45.3 Added another image with different value of EXIF Model, and extended MediaSourceImageTest with corresponding asserts to ensure each file has a correct EXIF data.

Also $type_id changed to $media_type_id to be consistent with #2831936-74: Add "File" MediaSource plugin.11

The last submitted patch, 46: 2831937-46-full.patch, failed testing.

l0ke’s picture

#2831936-74: Add "File" MediaSource plugin.15 Function createMediaTypeTest() have been renamed to doTestCreateMediaTypeiaType().

phenaproxima’s picture

Status: Needs review » Postponed

As per the roadmap (#2825215: Media initiative: Essentials - first round of improvements in core), this is postponed on #2831936: Add "File" MediaSource plugin since the Image source directly extends File.

seanB’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
51.7 KB
35.95 KB
5.17 KB

Reroll based on #2831936-96: Add "File" MediaSource plugin. Let's just keep up with the changes as much as we can to minimize efforts.

naveenvalecha’s picture

Quick look at the plugin-only patch file which could be taken care at next reroll.

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

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

  2. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -0,0 +1,248 @@
    +          /** @var \DateTime $date */
    

    Need small adjustment /** @var \Drupal\Core\Datetime\DrupalDateTime $date */

//Naveen

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

I think #50 was meant to be Needs review?

The last submitted patch, 35: media_image_plugin-2831937-35.patch, failed testing.

The last submitted patch, 39: media_image_plugin-2831937-39.patch, failed testing.

The last submitted patch, 41: 2831937-41.patch, failed testing.

seanB’s picture

Yeah, sorry about that!