Cleanup the use of custom item properties in EntityReferenceFormatterBase

+1 this broke der

@larowlan, can you elaborate a bit on what you mean by "this"? :)

The original entity bit

DER is https://drupal.org/project/dynamic_entity_reference and perhaps the related issue is #2366093: Unable to set field value programmatically..

It is a bug report as per #1 and contrib blocker so it is a major.

Sorry, yes when the original issue went in (#2346315: Translated entity references not rendered in the entity display language) it made the DER formatter need a similar change - I don't think we should need to set random object properties to get a formatter to work - so agree with the issue space here.

See http://cgit.drupalcode.org/dynamic_entity_reference/commit/?id=5e70ba5 for the DER commit.

Let's try it out then :) Just wondering if we'll get the same errors as #2370703-12: ER's "autocreate" feature is mostly broken (and untested) or more.

sorry for the noise :)

Created #2377841: Allow user to chose per entity type formatter. for DER after #7

I'll try to look at some fails. If I can wrap my head around it.

Let's see how much this fix. I have reverted the access change just to check the fails.

Some test fixes. Interdiff is against #11.

Here is the final fix hopefully green.

+++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
@@ -37,7 +37,7 @@ protected function getEntitiesToView(FieldItemListInterface $items) {
-      if ($entity->access('view')) {
+      if ($entity && $entity->access('view')) {

While viewing host entity after deleting referenced entity I was getting non-object error hence this fix. We are still setting $entity = $item->entity; which is NULL in this case. It fixes the issue but is it a correct fix? IMO EntityReferenceFormatterBase::prepareView() should not set it. And we don't have tests for this case in core luckily EntityCacheTagsTestBase tests this case.

This is a green patch. I want to say good night but it is almost noon here.

The need to explicitely grant "access content" to anon users in tests sure is a bit tedious. Wondering how we could make that less painful.

We can create a trait for that.

Not sure a trait with a single method will be very useful here, maybe just a helper method (with $roles and $permissions parameters) on our base test classes.

About these custom properties.. the patch in #11 was written just from curiosity, I'm sorry if it was seen as an approval of the issue scope :)

Now that I'm giving it more than a 2 second thought, I think that removing 'access' is probably fine. It is used mainly to simplify tests but also a static cache for access checking at render time, if the same ER field item is render multiple times. That static cache can be useful but I guess we can move it to a better place like the entity access handler, if it doesn't have one already.

However, 'originalEntity' was added with a very specific purpose in #2346315-23: Translated entity references not rendered in the entity display language. Since EntityReferenceFormatterBase::getEntitiesToView() returns potentially translated entities, I think it's necessary (or very useful?) to keep at hand the original (untranslated) entity too.

Thanks @yched and @amateescu for the review. So is it still a NR, NW or RTBC?

Not sure a trait with a single method will be very useful here, maybe just a helper method (with $roles and $permissions parameters) on our base test classes.

EntityReferenceAutoCreateTest extends WebTestBase
EntityReferenceFieldTranslatedReferenceViewTest extends WebTestBase
EntityReferenceFormatterTest extends EntityUnitTestBase
EntityReferenceRdfaTest extends EntityUnitTestBase
EntityViewBuilderTest extends EntityUnitTestBase

So in which base class perhaps TestBase?

Now that I'm giving it more than a 2 second thought, I think that removing 'access' is probably fine. It is used mainly to simplify tests but also a static cache for access checking at render time, if the same ER field item is render multiple times. That static cache can be useful but I guess we can move it to a better place like the entity access handler, if it doesn't have one already.

So this contradicts from IS

- getEntitiesToView() sets $item->access = TRUE as a static cache for "I already checked 'view' access on that entity, and it's TRUE, no need to check it again", in case the ER field is displayed several times in the request. However :
It's incomplete, only "view access is TRUE" is preserved, access gets re-computed each time if it's FALSE.
EntityAccessControlHandler already has its own static cache for access checks, so it seems useless and error-prone to have another caching logic somewhere else ?

Can we please have a consensus here?

However, 'originalEntity' was added with a very specific purpose in #2346315-23: Translated entity references not rendered in the entity display language. Since EntityReferenceFormatterBase::getEntitiesToView() returns potentially translated entities, I think it's necessary (or very useful?) to keep at hand the original (untranslated) entity too.

Do you want me to revert that change?

Reroll after formatter moved to core.

Not sure I get that - the patch here doesn't change the fact that $this->entity always contain the untranslated entity. The translated entities are only returned by getEntitiesToView(), and not written anywhere in the $item, that still only contains the untranslated one (which is why I don't see why originalEntity is needed to begin with)

Fair enough, I was talking from distant memories so that's why it probably didn't make sense. Now I'm looking at the actual code/patch and I see the following removed code:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
@@ -30,26 +30,15 @@ protected function getEntitiesToView(FieldItemListInterface $items) {
-      // The "originalEntity" property is assigned in self::prepareView() and
-      // its absence means that the referenced entity was neither found in the
-      // persistent storage nor is it a new entity (e.g. from "autocreate").
-      if (!isset($item->originalEntity)) {
-        $item->access = FALSE;
-        continue;
-      }

This comment looks to me quite explanatory on why EntityReferenceFormatterBase::prepareView() sets a custom property (originalEntity). Basically, prepareView() has to be performant and handle multiple-loading of referenced entities, but it should not unset any $item for which the referenced entity is not available anymore, so it has to signal this case somehow to the following code paths which have to loop over $items and decide if there's something to render or not.

What is the next step here now?

+1 for #39 :)

+        // (...) All items are initialized at FALSE.
+        $item->loaded = TRUE;

I just submitted the same but you were faster :D

Also, this means we can change the tests to set ->loaded = TRUE instead of all the things we're changing now?

Not sure if we really need to switch the translation *before* checking access(), BTW. Can entity access be granted or denied depending on the language the entity is in ?

I have exactly 0 clues about that :/ I'd say to just make sure we're doing it in the same order as HEAD.

No opinion on the _ prefix so the patch looks ready to go.

Can entity access be granted or denied depending on the language the entity is in ?

I just spoke to @plach in IRC about this and the answer is yes. So we're doing it right by checking access after getting the translation.

It's not entirely clear what bug is being fixed from the issue summary. Also can the beta evaluation be added. Thanks.

/**
 * Implements hook_install().
 */
function node_install() {
  // Enable default permissions for system roles.
  // IMPORTANT: Modules SHOULD NOT automatically grant any user role access
  // permissions in hook_install().
  // However, the 'access content' permission is a very special case, since
  // there is hardly a point in installing the Node module without granting
  // these permissions. Doing so also allows tests to continue to operate as
  // expected without first having to manually grant these default permissions.
  if (\Drupal::moduleHandler()->moduleExists('user')) {
    user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('access content'));
    user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array('access content'));
  }

That's how standard does it :)

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAutoCreateTest.php
@@ -36,6 +37,10 @@ class EntityReferenceAutoCreateTest extends WebTestBase {
+    $user_role = Role::load(DRUPAL_ANONYMOUS_RID);
+    $user_role->grantPermission('access content');
+    $user_role->save();

So I'm not sure why this is necessary since node is being installed. I think we need more investigation.

I agree this is test code let's not dwell about it.

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
@@ -119,6 +125,11 @@
+    // Revoke the 'view test entity' permission for this test.
+    Role::load(DRUPAL_ANONYMOUS_RID)
+      ->revokePermission('view test entity')
+      ->save();

This is a cleaver change. I like it.

It duplicates EntityAccessControlHandler's own static cache. Two static caches on top of each other is useless and error-prone.

I'm committing this under the beta fragility maintainer discretion proviso. Committed 6d75fd5 and pushed to 8.0.x. Thanks!