Problem/Motivation

If I create an entity reference field (not file field) that can reference files, I'm able to use the default 'Rendered entity' formatter. Problem is, for files, this renders nothing. Without a module like File Entity available, entity_view() does nothing for files.

A node with a entity reference field referencing a file:

No output visible

HTML output showing nothing:

We have to special case file entities in Entity Embed, but my concern is that other things may try to render entities that have a view builder class, but are not actually viewable.

Proposed resolution

Add a isViewable() method with corresponding property to \Drupal\Core\Entity\EntityType. I tried setting 'view_builder' => NULL in the annotation for \Drupal\file\Entity\File but it breaks rendering any base fields on files, which is used for the file listing page at /admin/content/file.

Remaining tasks

* Write change notice

User interface changes

None

API changes

This adds a 'viewable' property to \Drupal\Core\Entity\EntityType, default to FALSE. \Drupal\Core\Entity\ContentEntityType overrides this with TRUE since it also is responsible for adding the default view_builder handler. No change in behavior.

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because users are tricked into using a formatter that cannot be used.
Issue priority Not critical because most people will use file fields, but this affects contributed modules like Entity Embed.
Unfrozen changes Unfrozen because it solves something that never worked in the first place.
Disruption Disruptive for any contrib modules that had un-viewable entities that used ContentEntityType. Of my knowledge there are no such contrib modules, and core's file module is the only known example to me.
CommentFileSizeAuthor
#18 Selection_302.png70.02 KBDave Reid
#9 interdiff.txt1.04 KBDave Reid
#9 2567919-file-entity-disable-view-builder.patch4.32 KBDave Reid
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,641 pass(es). View
#6 2567919-file-entity-disable-view-builder.patch4.19 KBDave Reid
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#2 2567919-file-entity-disable-view-builder.patch494 bytesDave Reid
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,627 pass(es), 20 fail(s), and 0 exception(s). View
Selection_295.png26.34 KBDave Reid
Selection_294.png13.58 KBDave Reid
Selection_293.png12.1 KBDave Reid
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Dave Reid created an issue. See original summary.

Dave Reid’s picture

Status: Active » Needs review
FileSize
494 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,627 pass(es), 20 fail(s), and 0 exception(s). View
Dave Reid’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 2567919-file-entity-disable-view-builder.patch, failed testing.

The last submitted patch, 2: 2567919-file-entity-disable-view-builder.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Ok, so having a view_builder handler is important for building base fields, but not the entity itself. Therefore we should just add a 'viewable' property to EntityType so that the file class can override this.

Dave Reid’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 6: 2567919-file-entity-disable-view-builder.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,641 pass(es). View
1.04 KB

Also found that Views offers a 'File' entity row plugin which suffers the same problem. By adding a isViewable() method, it helps simplify this.

Also last patch was invalid, sorry.

The last submitted patch, 6: 2567919-file-entity-disable-view-builder.patch, failed testing.

Dave Reid’s picture

Issue summary: View changes
Dave Reid’s picture

Issue tags: +Media Initiative
Dave Reid’s picture

Priority: Normal » Major

Bumping priority as this is broken functionality in core.

Dave Reid’s picture

The redirect module will also want to use ContentEntity, but not have them as "viewable" and will need this functionality.

Dave Reid’s picture

chx’s picture

Do we want base entities to be not viewable by default....? If I remember correctly, for example, menu links are not content entities and yet viewable.

Dave Reid’s picture

Base entities are not viewable by default I thought, since they don't actually have a default value for view_builder. It's only ContentEntityType which adds the default view_builder class.

Dave Reid’s picture

FileSize
70.02 KB

The following Content Entity types are currently possible to render with the Views plugin, and demonstrates Files are not the only problem here. Note some of these are config entities that actually have view_builder classes (like Tour) and those are ok.

We probably would need to add the same restriction to Custom menu links and Shortcut links in addition to files. Neither of those actually render correctly.

Dave Reid’s picture

An alternative to this is to not merge in the default view_builder class in ContentEntityType and make each entity type declare its view_builder class itself (even if it's the default).

yched’s picture

Totally agree that ConfigEntities should not be considered viewable.
One weirdness being that Block config entities have a ViewBuilder...

Dave Reid’s picture

Issue tags: +API addition

Not sure how to get feedback on this from someone higher up on this... since it involves changing/adding an API and is a contributed project blocker.

yched’s picture

@Dave Reid : maybe ping @timplunkett ?

Wim Leers’s picture

Patch looks great!


ContentEntityType does this:

  public function __construct($definition) {
    parent::__construct($definition);
    $this->handlers += array(
      'storage' => 'Drupal\Core\Entity\Sql\SqlContentEntityStorage',
      'view_builder' => 'Drupal\Core\Entity\EntityViewBuilder',
    );
  }

i.e. it assigns the default entity view builder to every content entity type. Wouldn't you also want to change this then?


  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -666,6 +673,13 @@ public function isRevisionable() {
    +    return !empty($this->viewable) && $this->hasViewBuilderClass();
    

    Why not just $this->viewable?

    It defaults to FALSE so will always be set.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
    @@ -159,7 +159,8 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    return \Drupal::entityManager()->getDefinition($target_type)->isViewable();
    +
       }
    

    Nit: Extraneous newline here.

Dave Reid’s picture

Also, really strange that I can reference config entities (tours for example) with an entity_reference field, but since it provides a view builder class, I can select a formatter with a view mode. Problem is, they don't support view modes at all.

Dave Reid’s picture

I think we need some way of distinguishing three different kinds of viewable states of entities

  1. Entities that cannot be rendered
  2. Config Entities that can be rendered, but not using view modes
  3. Content entities that can be rendered

If I'm using an Entity Reference field with the 'Rendered Entity' formatter, the corresponding behavior for the above conditions should be:

  1. Entities that cannot be rendered: The formatter should not even be available.
  2. Config Entities that can be rendered, but not using view modes: The formatter should be available, but no view mode option should be visible. I guess it would be ok to only have 'Default' view mode as an option here as the current behavior. I think end users may be confused that they cannot create arbitrary view modes for these entity types.
  3. Content entities that can be rendered: Current behavior

Would it be possible a content entity that can be rendered but doesn't support view modes?

catch’s picture

Title: Should not be possible to use the 'Rendered entity' field formatter for files without file_entity » Entity reference tries to allow view mode formatter configuration for unviewable entity types
Component: file.module » entity system

Not sure the new title is perfect, but trying to get it a bit closer to the patch. Also moving to entity system.

I don't think the last question in #25 is possibly at the moment, so the 1/2/3 case should be all we need to deal with.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

xjm’s picture

Priority: Major » Normal
Issue tags: -Contributed project blocker +Contributed project soft blocker

The core committers and entity/field API maintainers discussed this issue today and thought that while this is an obvious bug (and silly), it does not need to be major because:

  1. You can't really run into it by itself in core without a contrib module.
  2. Contrib that is affected can already special-case it (so it seemed like more of a contrib soft blocker).
  3. Lacking that, site builders will quickly see the formatter does not work as expected and can select a different one (like the filename).

It's possible we've overlooked something though. @Dave Reid (or anyone else), if it's problematic to work around this in contrib, can you update the issue with that information and re-promote it to major? Thanks!

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.

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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