Currently, entity_view() uses an heuristic based on the path to determine whether the entity is viewed in page mode or not. It matches the way Drupal core does it, which has already been determined as a bad idea (see #721754: A node cannot be displayed in different view mode on its own page).

I suggest we allow this mode to be set by the caller, or automatically determined by view mode. After all, most of the time this is what you want (I would argue that the 'full' mode for core nodes has very little use outside a full page context).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
2.88 KB
4.58 KB

I see two options:

Option 1. Implement something close to what Drupal 6 was doing: add an explicit $page argument to entity_view(). The old behavior is kept when this argument is not present. We can even make this completely backward compatible if we don't change the EntityAPIControllerInterface interface (but I would recommend modifying it).

Option 2. Allow the view mode to explicitly say that they should be displayed in full page, by adding a new key to the view modes array.

I would recommend option 1. Option 2 seemed like a good idea when I wrote it, but I think leaving the caller decide is the best we can do.

Damien Tournoud’s picture

Title: Per view mode 'page' setting » Make the 'page' setting for entity view useful again
Damien Tournoud’s picture

FileSize
4.58 KB

Reuploading option 1 to fix a typo.

pounard’s picture

Entity view returns the build array, so Option 2 still let the caller decide, but Option 1 seems less confusing. Maybe the two could be done and optional, one overriding the other.

fago’s picture

Status: Needs review » Needs work

I agree that ideally the caller should be able to decide upon this.
With option2 one might end up with viewing in page-module if people re-use the view mode somewhere else, e.g. from a view. In most cases that's probably unwanted behaviour then, so let's require $page to be explicitly set (option1)

@patch:
* Problem is entity_view() also serves as harmonized entity_view() for core entities, where this option does not exist. So we'll need to document that this option only works for entities provided via the EntityAPIControllerInterface.
* Also, we need to document the default behaviour of guessing if NULL "is passed".
* It's a bit unfortunate to have another API change, but most people should use or extend the provided controller anyway - beside you.. ;) So I guess it should be ok.

For d8, I think we should migrate entity_view() to take a $options array like many other functions.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
5.37 KB

Rerolled, based on comments from #5.

fago’s picture

Status: Needs review » Fixed

Thanks, committed.

-> We have an API Change for EntityAPIControllerInterface::view():

Modules that override the method of the default controller or implement the interface themselves have to follow the following change:

-  public function view($entities, $view_mode = 'full', $langcode = NULL) {
-    $view = parent::view($entities, $view_mode, $langcode);
+  public function view($entities, $view_mode = 'full', $langcode = NULL, $page = NULL) {
+    $view = parent::view($entities, $view_mode, $langcode, $page);

Status: Fixed » Closed (fixed)

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

fago’s picture

Status: Closed (fixed) » Needs review
FileSize
569 bytes

AS #1210806: Add corollary to node_is_page() has shown, we forgot adding $page to Entity::view().

Doing so is an API change. However usually modules should override buildContent() not view(), so it should be ok.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Couldn't be more straightforward.

Damien Tournoud’s picture

Yes, sorry about that.

fago’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

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