Add "Image" MediaSource plugin

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

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

Rolling out tests.

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.

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.

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.

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

Here are added tests listed in #12.

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

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

Also default source field is provided.

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

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

Added default configurations and adjusted comments written in #21.

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.

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.

Should be ready to test...

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.

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.

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

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.

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

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

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

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

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

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

#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

Rerolled

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

Fixed everything from #40

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

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.

#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

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

#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

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

As per the roadmap (#2825215: Media initiative: Roadmap), this is postponed on #2831936: Add "File" MediaSource plugin since the Image source directly extends File.

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.

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

I think #50 was meant to be Needs review?

Yeah, sorry about that!

I think this is ready — this is simply blocked on #2831936: Add "File" MediaSource plugin going in. What could help in the mean time, is for the IS to be updated already.

@seanB has opened an issue in Media Entity Image to track and detail the upgrade path from that module to this source plugin: #2883452: Provide upgrade path for media_entity_image config changes to Drupal core. Adding it as a related issue.

#2831936: Add "File" MediaSource plugin landed!

In addition to an IS update (and possibly a change record), this will need a reroll on top of the committed File plugin, which saw some changes since #50.

Reroll (straight rebase) provided by git:

$ git apply -3v 2831937-50-plugin-only-do-not-test.patch
2831937-50-plugin-only-do-not-test.patch:22: trailing whitespace.
?PNG
Checking patch core/modules/media/config/schema/media.schema.yml...
Checking patch core/modules/media/images/icons/no-thumbnail.png...
Checking patch core/modules/media/src/Plugin/media/Source/Image.php...
Checking patch core/modules/media/tests/fixtures/exif_example_1.jpeg...
Checking patch core/modules/media/tests/fixtures/exif_example_2.jpeg...
Checking patch core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php...
Checking patch core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml...
Checking patch core/profiles/standard/config/optional/core.entity_view_display.media.image.default.yml...
Checking patch core/profiles/standard/config/optional/field.field.media.image.field_media_image.yml...
Checking patch core/profiles/standard/config/optional/field.storage.media.field_media_image.yml...
Checking patch core/profiles/standard/config/optional/media.type.image.yml...
Applied patch core/modules/media/config/schema/media.schema.yml cleanly.
Applied patch core/modules/media/images/icons/no-thumbnail.png cleanly.
Applied patch core/modules/media/src/Plugin/media/Source/Image.php cleanly.
Applied patch core/modules/media/tests/fixtures/exif_example_1.jpeg cleanly.
Applied patch core/modules/media/tests/fixtures/exif_example_2.jpeg cleanly.
Applied patch core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php cleanly.
Applied patch core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml cleanly.
Applied patch core/profiles/standard/config/optional/core.entity_view_display.media.image.default.yml cleanly.
Applied patch core/profiles/standard/config/optional/field.field.media.image.field_media_image.yml cleanly.
Applied patch core/profiles/standard/config/optional/field.storage.media.field_media_image.yml cleanly.
Applied patch core/profiles/standard/config/optional/media.type.image.yml cleanly.
warning: 1 line adds whitespace errors.
wim.leers at acquia in ~/Work/d8 on 8.4.x*

I did a reroll(at #1970534-143: Patch testing issue ) with some of the changes from #2831936: Add "File" MediaSource plugin and working on fixing fails now.

This reviews all code except for the test coverage. Still to review: test coverage and UI.

Addressed all my remarks except points 4, 5, 10, 11 and 12.

1. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   + * Image entity media source.
   ...
   + *   description = @Translation("Use local images for reusable media."),
   

   Consistent with the File plugin. Great.

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

   string

3. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   +      static::METADATA_ATTRIBUTE_WIDTH => $this->t('Width'),
   +      static::METADATA_ATTRIBUTE_HEIGHT => $this->t('Height'),
   ...
   +        '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'),
   

   Why do we only have class constants for the first two?

4. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   +          $date = new DrupalDateTime($this->getExifField($uri, 'DateTimeOriginal'));
   

   This calls the DrupalDateTime constructor without a $timezone parameter. Which means it uses the system timezone. Is this correct?

   (I can imagine it would need to be UTC always. I could also imagine it needs to be converted to the current user's timezone.)

   I think we want test coverage to prove that when we expose this metadata of a source=Image Media entity by mapping it to a field, then that field needs to show it in the current user's timezone.

5. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   +    if (!$this->canReadExifData()) {
   +      $form['no_exif_data_reader'] = [
   +        '#markup' => $this->t('Unable to read EXIF data for images. In order to provide EXIF data reading functionality please take a look at PHP documentation of the <a href="https://secure.php.net/exif_read_data" target="_blank">exif_read_data</a> function.'),
   +      ];
   +    }
   

   What are the consequences when I:

   1. create a module/install profile/custom site
   2. map some EXIF metadata to a field, have this be part of the default (or even optional) configuration
   3. but then the server cannot read EXIF data

   I think we want test coverage for this edge case.

   (Sadly config cannot express dependencies on certain environment capabilities.)

6. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   +   * @param string $field
   +   *   The name of the requested EXIF field.
   

   $exif_field, to make it super explicit that this is NOT a Drupal field.

7. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   +   *   The URI for the file that we are getting the EXIF field value.
   

   s/value/values/, because we're getting all values.

8. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -0,0 +1,248 @@
   +  protected function getExifData($uri) {
   

   Does not correspond to the docblock.

9. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
   @@ -0,0 +1,94 @@
   + * @package Drupal\Tests\media\FunctionalJavascript
   

   rm.

10. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
    --- /dev/null
    +++ b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
    
    +++ b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
    --- /dev/null
    +++ b/core/profiles/standard/config/optional/core.entity_view_display.media.image.default.yml
    
    +++ 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
    

    Must be moved to media module, for same reasons as we did that for file.

    However, manually differed this with the file config, and confirming it's consistent.

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

    This is inconsistent with file, which was required, not optional.

12. +++ b/core/profiles/standard/config/optional/media.type.image.yml
    @@ -0,0 +1,14 @@
    +dependencies:
    +  module:
    +    - media
    

    media.type.file doesn't list any dependencies. Not sure what's correct here.

    And why doesn't this also list image explicitly?

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

    Let's use the simpler description, like we did for file.

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

    true, like file.

The UI works consistently with how files work. Everything is set up automatically by installing the Media module. Except that the default "full" view display of an image media entity doesn't make a whole lot of sense:

\Drupal\Tests\media\FunctionalJavascript\MediaSourceImageTest is failing because it's still expecting that the mime and size attributes exist. They used to be inherited from the File Media Source plugin, but that was removed in #2831936-141: Add "File" MediaSource plugin.

Even if I comment those, the test is failing. It sounds like @vijaycs85 is working on fixes, which is wonderful :) It means I can focus on actually reviewing the test coverage.

The test coverage looks fine to me. But I'd like to see it expanded to cover what I pointed out in #63.5.

So, NW for:

1. not yet passing tests (see #62 + #66)
2. #63 points 4, 5, 10, 11 and 12.
3. #63.5 is about tests, so adding Needs tests
4. If #2831936: Add "File" MediaSource plugin needed a CR, then so does this. I'll leave it up to others to decide whether we should expand https://www.drupal.org/node/2883488 or create a new one.

All in all, this is already very close :)

This should fix the broken tests. The problem was that Image was inheriting createSourceField() from its parent class, which was setting a bunch of non-image extensions as acceptable.

#68 solved #67.1.

That still leaves points 2, 3 and 4 in #67.

Updated IS to make its "remaining tasks" section reflect those in #67.

For #67.2 and #67.3:

• #63.4 The exif specification doesn't seem to define a standard for timezones. Using the timezone of the current user seems to make the most sense. DrupalDateTime already uses the timezone of the current user when nothing is passed (system timezone only if the user timezone is not configurable). See drupal_get_user_timezone(). I tried to fix this, but there is a problem. Users don't need to define a date field to store the value. They can store it as a string or integer field as well. Since the storage timezone for datetime is UTC, this means you want to return a DATETIME_DATETIME_STORAGE_FORMAT formatted string converted from the user timezone to DATETIME_STORAGE_TIMEZONE for datetime fields, but you want to return something else for other field types (maybe a timestamp for integer fields and a custom format for string fields). I guess this is what #2862467: Add complex field mapping to media module is for. Since we currently only support simple mappings, I would suggest fixing date field handling in #2862467: Add complex field mapping to media module, and for now just support storing the date as a string in the default 'medium' format.
• #63.5 The method getMetadata also has a check for canReadExifData(). This means the metadata is not fetched when the server can't read exif data. I added a test image source that returns FALSE for canReadExifData() to test this.
• #63.10 Done
• #63.11 Done
• #63.12 Added dependency for image, also added dependencies for media.type.file.yml

Also expanded the CR for #67.4.

Added #2862467-5: Add complex field mapping to media module to make sure we discuss/fix date field handling for metadata.

#63.4: Hm… I think it's okay to defer this to a follow-up, but I'm not sure the change you made related to this makes sense. See code review below.
#63.5: the concern is not what metadata attribute value is returned (I agree that'd simply fall back to FALSE), but how this would work for the field that this is mapped to. EDIT: this is addressed, see below.

1. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -207,7 +219,7 @@ public function getMetadata(MediaInterface $media, $name) {
   -          return $date->getTimestamp();
   +          return $this->dateFormatter->format($date->getTimestamp());
   

   I think this change is actually wrong. Because the value returned by getMetadata() is what will get stored in the field that this is mapped to (if there's any).

   Consequently, that means we're storing a user-timezone-adjusted timestamp and using it as the canonical timestamp. That seems wrong.

2. +++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
   @@ -45,6 +45,36 @@ public function testFileMediaRevision() {
   +  public function testImageMediaRevision() {
   

   Good call to add this!

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
   @@ -80,4 +124,55 @@ public function testMediaImageSource() {
   +    // mapped to EXIF data should be null.
   +    $media = Media::load(1);
   +    $this->assertEquals('exif_example_1.jpeg', $media->label());
   +    $this->assertEquals('200', $media->get('field_string_width')->value);
   +    $this->assertNull($media->get('field_string_model')->value);
   +    $this->assertNull($media->get('field_string_iso')->value);
   

   This is the key thing that we're testing. That addresses my concern in #63.5, thanks!

P.S.: I think the revision test that you added failed because you're only uploading a file, you're not filling out the Alternative text input :)

This should fix the tests.

Thanks @phenaproxima. So that takes care of the tests. The attached patch fixes #73.1. Discussed this with WimLeers on IRC. Date handling is going to be fixed in #2862467: Add complex field mapping to media module. Changed the date display from the medium date format to RFC3339, since that format is OK to read, contains timezone information and is easier to handle for machines. Also added a @todo and some documentation in the image source.

This means everything in #63, #67 and #73 should now be addressed. If WimLeers could confirm this then we could update the IS and hopefully RTBC.

Not sure why it bothered me so much, but I felt the need to fix the single spaces between braces in our config. That seems to be the standard (did not know that). I will fight the urge to create another issue for the other 24 occurrences in core.

#74:

+++ a/core/modules/media/config/install/media.type.file.yml
@@ -1,9 +1,6 @@
+dependencies: {  }
-dependencies:
-  module:
-    - file
-    - media

+++ b/core/modules/media/config/install/media.type.image.yml
@@ -1,9 +1,6 @@
-dependencies:
-  module:
-    - image
-    - media
+dependencies: {  }

Are you sure these changes are correct?

#76: HAHAHAH :D I noticed that too, but I explicitly didn't change that :P

Tests were added. CR was updated.

Re-reviewing the code, this is the only thing that comes up:

+++ b/core/modules/media/src/Plugin/media/Source/Image.php
@@ -0,0 +1,310 @@
+          // @todo: We currently only support simple mapping for string fields.
+          // This needs to be re-evaluated when complex mapping for date fields
+          // is added https://www.drupal.org/node/2862467. For now we use a
+          // RFC3339 formatted date since it contains all information including
+          // the timezone.
+          /** @var \DateTime $date */
+          $date = new DrupalDateTime($this->getExifField($uri, 'DateTimeOriginal'));
+          return $date->format(\DateTime::RFC3339);

I'm still concerned how the DateTimeOriginal data in EXIF is interpreted. I did some more research. It seems that there is also an optional Timezone EXIF field that specifies the timezone explicitly. See http://www.exiv2.org/tags.html. Its docs say:

This optional tag encodes the time zone of the camera clock (relativeto Greenwich Mean Time) used to create the DataTimeOriginal tag-valuewhen the picture was taken. It may also contain the time zone offsetof the clock used to create the DateTime tag-value when the image wasmodified.

I'm fine with deferring this to a follow-up. But I think it does need a follow-up.

Note that no perfect solution is possible: https://discussions.apple.com/message/18913151#18913151 + https://forums.adobe.com/thread/1562041. But at least we can do better for those cases where we do have timezone information.

IOW: this is very close :)

Are you sure these changes are correct?

DefaultConfigTest fails without them.

Opened a follow-up for the EXIF timezone stuff: #2885729: Smooth out timezone handling when retrieving date/time info from EXIF

#79: hm, maybe, but that doesn't mean this is correct. However, this patch is then just consistent with #2831936: Add "File" MediaSource plugin. I think ideally we'd have a follow-up to investigate those config dependencies though, because ideally those MediaType config entities would depend on those modules.

But again: that should not hold up this issue. So, given that all my feedback has been addressed: back to RTBC! Great job :)

Updated IS to list the follow-ups.

New separated the CR as discussed with GaborHojtsy on IRC.

Reviewing this for a potential commit. Noted the following:

• The change record is already published. Looks like the existing one for file was updated. Discussed this on the media meeting that for visibility in the changes feed / for people following what changed, etc. it would be better to have a separate change record (draft).
• Not introduced by this patch but I found while reviewing that the media view includes the bulk form actions column, which does not work. That is because the actions were removed from the initial media entity patch but not from the view. So now the view and the media views config schema has the media_bulk_form plugin but no implementation exists. Opened #2886178: Media view includes media_bulk_form but no implementation and marked as related to #2877383: Add action support to Media module.
• We had an issue with the file media type vs. the fixed list of files allowed. For this image one, we have the same types as for image fields, but again these are not configurable. There is more generally #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) but ultimately #806102: Locked fields can change the widget but not settings applies to this also.
• Wim raised the media full view looked bad. It still pretty much looks the same, the field display label is fixed. I think displaying the thumbnail in this view looks odd and then the label on the image looks odd. But the file media full view is also pretty much the same (with thumbnail, etc). Also people will likely not view this page much. So IMHO we can defer to tinkering with this as it is default configuration pretty much.
• Finally, and this is the main thing I found, I was wondering how the EXIF data is extracted. We hardcode checking for the existence of a PHP function and then point to the PHP docs in the error message and use the PHP function directly to get EXIF data. This sounds like a classic use case for dependency injection to me because we are using a service effectively. If we inject an EXIF service that allows people on hosts where they cannot change the PHP extensions/binaries to install a module that implements EXIF parsing in userspace. Or if in the future better EXIF reading emerges as a PHP library (as opposed to what is built into PHP) then people can use that easily. That is already possible by extending the image class but then it will be a different media bundle, etc. If we make it an injectable service then we open the door for contrib / custom builds to get creative. This is totally in the realm of core framework managers (and to a lesser extent release managers to help define priority) so I would defer to them for feedback on this.

Otherwise looks good to me :)

#85: RE: EXIF: note that I already pointed this out in #63.5. Explicit test coverage for this was added. I think the concern isn't as broad, because AFAIK the vast majority of PHP installations have it compiled.

@seanB and I discussed on IRC and I think we've reached agreement that, right now, EXIF is more trouble than it's worth. We've decided to pull that out of core, at least for the time being, and put it into Media Entity Image's 8.x-2.x branch in contrib, which will be built on top of core Media. So EXIF will still be supported, backwards compatibility will be maintained, and core can stay simple and not deal with the jankiness. Everybody wins.

So we'll need to update our new change record for that. I will post a patch soon with all the EXIF stuff gone.

:O I don't get this though. I was fine with the EXIF state in the current patch. We even have test coverage for the case where EXIF support disappears from under you.

For the record I did not advocate for removal of EXIF support, but I can see *if* the effort is too much to make it a service then this may be the easier path for now.

Or if in the future better EXIF reading emerges as a PHP library (as opposed to what is built into PHP) then people can use that easily.

For the record, the PHP Exif Library provides a full PHP based solution to read/write EXIF data from/to files (i.e. it does not require PHP EXIF extension). In the contrib space, the File metadata manager module integrates that library as a plugin into a service.

(But, TBH, PEL is not currently implementing EXIF/TimeZoneOffset tag).

Extra:
Are we managing the image files like PNG that doesn't have EXIF correctly? Apparently we are always trying to read exif data and attaching empty EXIF properties.

The EXIF read/write/support should not be restricted to supported formats? JPG and TIFF

+  protected function canReadExifData() {
+    return function_exists('exif_read_data');
+  }

+    if ($this->canReadExifData()) {
+      $attributes += [
+        static::METADATA_ATTRIBUTE_MODEL => $this->t('Camera model'),
+        static::METADATA_ATTRIBUTE_CREATED => $this->t('Image creation date & time'),
+        static::METADATA_ATTRIBUTE_ISO => $this->t('ISO'),
+        static::METADATA_ATTRIBUTE_EXPOSURE => $this->t('Exposure time'),
+        static::METADATA_ATTRIBUTE_APERTURE => $this->t('Aperture value'),
+        static::METADATA_ATTRIBUTE_FOCAL_LENGTH => $this->t('Focal length'),
+      ];
+    }

http://php.net/manual/en/function.exif-read-data.php

+  protected function getAllExifFieldValues($uri) {
+    return exif_read_data($this->fileSystem->realpath($uri), 'EXIF');
+  }

See, stuff like this is why EXIF is confusing and contentious, and better off not being in core for the time being.

Here is the patch without any EXIF stuff at all. I'll update the CR and open a follow-up in Media Entity Image for re-adding EXIF support.

Follow-up filed against Media Entity Image: #2886552: Add EXIF support on top of core Image plugin. Also updated the change record to reflect that.

I can't re-RTBC without that follow-up.

Plus, there's still many mentions of "EXIF" left in this patch.

Follow-up added in #93, although it is against Media Entity Image. I'm not sure what updates are needed to the change record, though; I already referenced that issue there.

Here is the patch, re-rolled without any mention of EXIF...except in the example images, which still contain EXIF tags. I figure that's pretty harmless, and it'll give Media Entity Image some useful files to test with.

For some reason, the interdiff I got was insanely wonky -- all kinds of weird stuff that did not, in fact, change between patches. So I'm punting on the interdiff.

Patch looks good. Reviewed the svgs in the patch which needs removal of spaces and EOL which does not look blocker to me.

//Naveen

@naveenvalecha: while it can apply, it is not a good idea to commit this with the whitespace problem unfortunately:

$ git apply --index 2831937-96.patch 
2831937-96.patch:258: trailing whitespace.
?PNG
warning: 1 line adds whitespace errors.

Here's the patch which fixed the whitespace. didn't get the interdiff for it.

//Naveen

All right, we discussed with @phenaproxima that git believing that there is a newline that should not be in a patch does not mean the PNG is wrong. Manually checking the PNG (and other images) they look good.

Since all the concerns I raised and others raised were resolved and followups were opened, it was time to get this in :) Thanks all for your tireless efforts and congrats on another milestone reached. Let's meet in the widgets and formatters issues.

(Edit: committed #96).

@naveenvalecha: as per IRC discussion, you did not roll the patch with --binary which is why you did not get the warning. On the other hand the PNG would still be broken in that case as it was before as per above testing. So let's leave it as-is for now and resolve in a followup issue if it becomes a problem.

#103
Sure, Added a follow-up #2886856: Fix the whitepsace in png introduced by #2831937 to validate if it's not a problem.

//Naveen

WOOT! This means #2831943: Use "rendered media" (not links) as default media field formatter; add modal to configure the used media view mode and #2831941: [PP-1] Re-create Image field widget on top of media entity are unblocked!

And of course #2886552: Add EXIF support on top of core Image plugin.

Re-tagging.