Problem/Motivation

If I create an entity reference field (not file field) that can reference files, I cannot use any file field formatters to output the referenced files. Similar situation for image field formatters. Or for using entity reference field formatters on file and image fields.

Contributed modules like https://www.drupal.org/project/file_image_formatters and https://www.drupal.org/project/file_entity_reference_image_formatter and https://www.drupal.org/project/entity_formatters. This is easily solvable from core.

Proposed resolution

Allow reuse between file, image, and entity_reference field formatters.

If a field extends another field, it should ensure that the parent field's formatters are available to it as well. And in the case of entity-specific reference fields that extend the entity_reference field type, the entity-specific formatter should be available to entity_reference if the target_type matches the specific entity type.

Remaining tasks

- Write tests
- Needs change notice

User interface changes

- None (aside from additional formatters available in Field UI).

API changes

- None

Data model changes

- None

Comments

Dave Reid created an issue. See original summary.

Dave Reid’s picture

Status: Active » Needs review
FileSize
3.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,912 pass(es). View
Dave Reid’s picture

Title: Not possible to use any file field formatters for entity reference fields that reference files » Not possible to reuse field formatters between entity_reference, file, and image fields
Issue summary: View changes
Dave Reid’s picture

FileSize
80.36 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Expanding scope and allowing entity_reference field formatters to be used on file and image fields.

Dave Reid’s picture

FileSize
5.54 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,912 pass(es). View
2 KB

Invalid last patch.

The last submitted patch, 4: 2568289-reuse-file-formatters-on-entity-reference.patch, failed testing.

Dave Reid’s picture

Related issues: +#2567919: Entity reference tries to allow view mode formatter configuration for unviewable entity types
FileSize
5.68 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,909 pass(es). View
2.06 KB

Adding support for using file field formatters on image fields.

Dave Reid’s picture

Related issues: +#2568065: Drupal 8 Port
Dave Reid’s picture

FileSize
8.46 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,016 pass(es). View
2.78 KB

One more step to make it possible to use image field formatters on entity_reference and file fields. I think this makes a statement that if you have any field that extends another field, you *should* make sure your formatters can be used on the parent field as well, where it makes sense.

Dave Reid’s picture

Issue summary: View changes
Dave Reid’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

This makes perfect sense. I could only find a small issue that can be fixed on commit:

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
@@ -48,7 +48,33 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
+          // @todo Setting width and height manually could be avoided if these were available on the file entity itself.

We should wrap this comment to 80 chars.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2568289-reuse-file-formatters-on-entity-reference.patch, failed testing.

amateescu’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Reviewed & tested by the community
FileSize
8.5 KB

Rerolled and fixed #12.

amateescu’s picture

Issue tags: +DCTransylvania

I demoed this reroll at the DCTransylvania sprints :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs change record
  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
    @@ -39,4 +46,13 @@ protected function checkAccess(EntityInterface $entity) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +    // Since file formatters may be re-used on entity reference fields, ensure
    +    // that a file entity type is being referenced by the field.
    +    return $field_definition->getSetting('target_type') === 'file';
    +  }
    

    Is this tested?

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -47,7 +47,34 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
    +    // If this formatter is being used on a non-image field, ensure only images
    +    // are rendered, and have width and height properties set.
    +    if ($this->fieldDefinition->getType() !== 'image') {
    +      foreach ($entities as $delta => $entity) {
    +        $image = \Drupal::service('image.factory')->get($entity->getFileUri());
    +        if ($image->isValid()) {
    +          $item = $entity->_referringItem;
    +          $properties = $item->getProperties();
    +          // @todo Setting width and height manually could be avoided if these
    +          //   were available on the file entity itself.
    +          //   @see https://www.drupal.org/node/1448124
    +          if (!isset($properties['width']) || $item->get('width') === NULL) {
    +            $item->set('width', $image->getWidth());
    +          }
    +          if (!isset($properties['height']) || $item->get('height') === NULL) {
    +            $item->set('height', $image->getHeight());
    +          }
    +        }
    +        else {
    +          // Files not supported as images should not be displayed.
    +          unset($entities[$delta]);
    +        }
    +      }
    +    }
    

    This code seems untested?

Hmmm... the issue summary suggests both tests and a change record are need.

claudiu.cristea’s picture

What I don't like at this patch is that is hardcoding in the parent formatter the child field types. IMO, we should find a generic solution that applies to all fields based on the next statement: If a field extends another field, it should ensure that the parent field's formatters are available to it as well. The reverse cannot be implemented generically, so probably needs hardcoding in annotations for entity reference - file - image

Berdir’s picture

#17 sounds interesting, subclassing field types is I think a pretty common use case, now that it is so easy (example use case: A reference field with a description). Currently you always have to alter the existing types. I think we should have a way to opt-out of that, as we are adding it now, maybe it needs to be opt-in now, not sure?

And if we decide against that, I think file/image should alter the field formatter definitions themself and not hardcode them. That's also what telephone is doing for example.

delacosta456’s picture

hi

please can anyone help for the case of drupal 7

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
14.07 KB
15.12 KB

Added the tests required in #16 and changed the way we make these formatters available everywhere to hook invocations by their respective modules instead of changing the annotations.

If a field extends another field, it should ensure that the parent field's formatters are available to it as well.

Doing this would be an API change, because we would be switching from the current opt-in behavior (i.e. a formatter must provide the list of field types that can be used with it), to an opt-out behavior (i.e. a formatter must exclude a field type from its allowed list of compatible field types).

Status: Needs review » Needs work

The last submitted patch, 21: 2568289-21.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
14.07 KB
779 bytes

Fixed the fail and wrote a small CR: https://www.drupal.org/node/2801127

dawehner’s picture

+++ b/core/modules/file/file.module
@@ -49,6 +49,26 @@ function file_help($route_name, RouteMatchInterface $route_match) {
+  $info['file_default']['field_types'][] = 'image';
...
+  $info['file_table']['field_types'][] = 'image';
...
+  $info['file_url_plain']['field_types'][] = 'image';
...
+  $info['file_rss_enclosure']['field_types'][] = 'image';

Should we have a condition whether the image module is enabled? Alternative the image module could add those as well

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/file.module
    @@ -49,6 +49,26 @@ function file_help($route_name, RouteMatchInterface $route_match) {
    +function file_field_formatter_info_alter(&$info) {
    +  // Allow the file formatters to be used on entity reference and image fields.
    +  $info['file_default']['field_types'][] = 'entity_reference';
    +  $info['file_default']['field_types'][] = 'image';
    

    Agreed. the image part should be moved to image module. And the entity_reference part can be added directly in the annotations.

  2. +++ b/core/modules/file/file.module
    @@ -49,6 +49,26 @@ function file_help($route_name, RouteMatchInterface $route_match) {
    +  // Allow entity reference formatters to be used on file fields.
    +  $info['entity_reference_entity_id']['field_types'][] = 'file';
    +  $info['entity_reference_entity_view']['field_types'][] = 'file';
    +  $info['entity_reference_label']['field_types'][] = 'file';
    

    view doesn't work in core, but works with e.g. file_entity. I think that hides itself automatically without view mode, so that should be fine. Possibly open a follow-up in file_entity to remove it there then.

    doesn't label duplicate an existing one in file.module?

  3. +++ b/core/modules/image/image.module
    @@ -98,6 +98,20 @@ function image_help($route_name, RouteMatchInterface $route_match) {
    +  // Allow the image formatter to be used on entity reference and file fields.
    +  $info['image']['field_types'][] = 'entity_reference';
    +  $info['image']['field_types'][] = 'file';
    

    same this can also go directly on the annotation.

  4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -21,7 +21,9 @@
      *   label = @Translation("Image"),
      *   field_types = {
    - *     "image"
    + *     "entity_reference",
    + *     "file",
    + *     "image",
    

    apparently we already do, then the part above in the alter hook doesn't do anything?

  5. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -42,7 +42,34 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
    +    if ($this->fieldDefinition->getType() !== 'image') {
    +      foreach ($entities as $delta => $entity) {
    +        $image = \Drupal::service('image.factory')->get($entity->getFileUri());
    +        if ($image->isValid()) {
    

    this is problematic, we just recently fixed that in entity_embed. this means we could for example load a 400MB video file into memory, just to figure out if it is an image. That's extremely slow, especially if that file is on NFS or some other shared file system.

    What we did is check that the mime type starts with image/ and only then try to load it.

  6. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -42,7 +42,34 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
    +          // @todo Setting width and height manually could be avoided if these
    +          //   were available on the file entity itself.
    +          //   @see https://www.drupal.org/node/1448124
    +          if (!isset($properties['width']) || $item->get('width') === NULL) {
    +            $item->set('width', $image->getWidth());
    +          }
    

    and even if we only do it on images, loading them is quite slow and is something we so far tried to avoid for normal requests.

    Maybe we should avoid this part of the page and first think about getting the referenced issue in? file_entity has a flexible metadata system that can store arbitrary additional values pretty easily.

Berdir’s picture

There is another issue here.

I noticed this today: #2822429: template_preprocess_responsive_image() does unnecessary IO by creating Image objects. And noticed that my changes didn't work for us because we already do what this issue proposes. use an image formatter on a file field. But then there's no width/height metadata. Which would also be the case here, for responsive and non-responsive I believe.

I also opened #2822444: Provide width and height to image templates, which we might need in core to be able to provide width/height for those templates.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

diqidoq’s picture