If an entity reference field is refers to an entity, which is not available (accessible) for everyone and in one page request the host entity is viewed and saved, then the protected item in the entity reference field is get lost.

Step to reproduce:

  1. Install a new D8 site with standard installation profile
  2. Enable the entity_reference module
  3. Add a new entity reference field to node:article - ?q=admin/structure/types/manage/article/fields
  4. Ensure the field is editable ?q=admin/structure/types/manage/article/form-display
  5. Make the field visible - Choice the "label" formatter here ?q=admin/structure/types/manage/article/display and ?q=admin/structure/types/manage/article/display/teaser
  6. Create 3 article content with the following titles (to easily identify them)
    1. Child 01 published
      nid: 1
      Leave empty the entity reference field
    2. Child 02 UNpublished
      nid: 2
      Leave empty the entity reference field
    3. Parent
      nid: 3
      and refers to the "Child 01" and "Child 02"
  7. Enable the field_item_lost module. See below.
  8. Go to ?q=node/3/field-item-lost
  9. Open an incognito window in your browser and go to Go to ?q=node/3/field-item-lost
  10. Switch back to the logged in browser window and see again the ?q=node/3/field-item-lost

field_item_lost.module

<?php
/**
* @file
* Reproduce the bug.
*/
/**
* Implements hook_menu().
*/
function field_item_lost_menu() {
 
$items = array();
 
$items['node/%node/field-item-lost'] = array(
   
'title' => 'Field item lost',
   
'page callback' => 'field_item_lost_test_view_modes_pages',
   
'page arguments' => array(1),
   
'access callback' => TRUE,
   
'type' => MENU_LOCAL_TASK,
   
'weight' => 99,
  );
  return
$items;
}
/**
* Menu page callback.
*
* @param \Drupal\node\Entity\Node $node
*/
function field_item_lost_test_view_modes_pages($node) {
 
$view_modes = array('teaser', 'full');
 
$return = array();
  foreach (
$view_modes as $view_mode) {
   
$return[$view_mode] = entity_view($node, $view_mode);
  }
 
$node->save();
  return
$return;
}
?>

field_item_lost.info.yml

name: 'Field item lost'
type: 'module'
description: 'Reproduce the bug.'
package: 'Development'
core: '8.x'
version: '8.x-1.x'

This module just save the entity immediately after when the entity gets rendered.

You can find the reason in the \Drupal\entity_reference\Plugin\field\formatter\EntityReferenceFormatterBase::viewElements()

The unset() is operate on the live parts of the original entity.

Files: 
CommentFileSizeAuthor
#28 2078155-er-no-unset-just-check-access-28.patch7.7 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 59,197 pass(es).
[ View ]
#28 interdiff-28.txt966 bytesAmitaibu
#20 2078155-er-no-unset-just-check-access-20.patch7.7 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 59,062 pass(es).
[ View ]
#20 interdiff-20.txt902 bytesAmitaibu
#18 2078155-er-no-unset-just-check-access-17.patch7.71 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 59,024 pass(es).
[ View ]
#17 2078155-er-no-unset-just-check-access-16.patch7.98 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 58,616 pass(es).
[ View ]
#12 drupal-2078155.patch19.41 KBSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 58,070 pass(es).
[ View ]
#9 drupal-2078155.patch14.7 KBSweetchuck
FAILED: [[SimpleTest]]: [MySQL] 58,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#6 2078155-er-no-unset-just-check-access-6.patch4.71 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 58,416 pass(es).
[ View ]

Comments

I have a question:

The \Drupal\entity_reference\Plugin\field\formatter\EntityReferenceIdFormatter::viewElements() why does not filter the items by access, like every other EntityReference formatter does?

> The \Drupal\entity_reference\Plugin\field\formatter\EntityReferenceIdFormatter::viewElements() why does not filter the items by access, like every other EntityReference formatter does?

Opened #2078529: \Drupal\entity_reference\Plugin\field\formatter\EntityReferenceIdFormatter::viewElements() should call parent::viewElements()

I can confirm the bug in the OP.

Seems that it is not related to the formatter. Using your nodes from the OP, I have this inside hook_page_build:

<?php
function foo_page_build() {
 
$node = node_load(3);
 
// Return 1, as node ID 2 is unpublished.
 
dpm($node->field_er->count());
 
// The saved node will no longer have node ID 2.
 
$node->save();
}
?>

So to reproduce, you don't need the module from the OP, the above lines are enough.

Sorry, #4 is wrong, I was checking it on the node view itself. Here's the updated code:

<?php
function foo_page_build() {
 
$node = node_load(3);
 
// Returns 2 values.
 
dpm($node->field_er->getValue());
 
$view_modes = array('teaser', 'full');
  foreach (
$view_modes as $view_mode) {
   
$return[$view_mode] = entity_view($node, $view_mode);
  }
 
// Error -- Returns 1 value.
 
dpm($node->field_er->getValue());
}
?>

Status:Active» Needs review
StatusFileSize
new4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,416 pass(es).
[ View ]

Patch removes the unsetting of the values, and instead checks $item->access. Also renames a wrongly used variable.

@Amitaibu: I would like to practice the test writing, please do not write tests.

> @Amitaibu: I would like to practice the test writing, please do not write tests.

Great, thanks!

StatusFileSize
new14.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Here is the patch for test. I start to fix the formatters.

Status:Needs review» Needs work

The last submitted patch, drupal-2078155.patch, failed testing.

https://qa.drupal.org/pifr/test/613318
Looks good. Only the tree our error occurred.

Status:Needs work» Needs review
StatusFileSize
new19.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,070 pass(es).
[ View ]

Merge patches from #6 and #9.

Status:Needs review» Needs work

Thanks for working on this. Test is a bit different to what you usually find in core but looks well written and is understandable.

  1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,394 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {

    There is no parent implementation it could inherit the docs from, current standard is to leave that out.

  2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,394 @@
    +  public static $modules = array('node', 'entity_reference', 'entity_reference_test');

    This should be above the getInfo() and have a docblock, see other tests.

    All properties should be above the methods.

  3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,394 @@
    +          'entity_types' => array(),

    entity_types is no longer necessary.

  4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,394 @@
    +   * @param \Drupal\core\Entity\EntityInterface $node
    +   * @param string $account
    +   * @param array $expected

    Missing description of the method and the params.

    If it's really a $node, type hint as Drupal\node\NodeInterface. Not just in the @param, also in the method.

    $account as a string is a bit uncommon, $account is usually an object.

  5. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,394 @@
    +  protected function assertNodeView($node, $account, $expected) {

    assert*() methods are special in Simpletest, they are ignored when the line number is printed, so all these assertions will be printed. I that's what you want, fine, I guess it could make sense here, as we have the information in the message but often, that's not desired.

  6. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,394 @@
    +   * @param array $field
    +   * @param SimpleXMLElement[] $xpath_result
    +   * @param string $account
    +   * @param array $expected
    +   */
    +  protected function assertFormatterLabel($field, $xpath_result, $account, $expected) {

    Same.

    We isually don't use the Object[] syntax, to avoid arguments over it, I'd recommend "array".

  7. +++ b/core/modules/entity_reference/tests/modules/entity_reference_test/entity_reference_test.module
    @@ -4,3 +4,60 @@
    +function entity_reference_test_menu() {
    +  $items = array();
    +
    +  $items['node/%node/entity-reference-test-field-formatter'] = array(
    +    'title' => 'Test Entity reference field formatter',
    +    'page callback' => 'entity_reference_test_field_formatter_page',
    +    'page arguments' => array(1, 3),
    +    'access callback' => TRUE,
    +    'type' => MENU_LOCAL_TASK,
    +    'weight' => 99,
    +  );
    +
    +  return $items;

    Not exactly sure this is necessary (We could also just re-configure the default view mode and visit node/$nid) but if it is, you will have to convert this to a route.

    And if we really want to go that way, it might be better to use the entity_test test etity instead of node.

About the hook_menu().

Not exactly sure this is necessary (We could also just re-configure the default view mode and visit node/$nid) but if it is, you will have to convert this to a route.

A special page callback is necessary, because we have to call the $entity->save() method right after the entity_view(), to make the changes permanent what the field formatters does.

<?php
$entity
= entity_load($entity_type, 1);
entity_view($entity, $view_mode);
$entity->save();
?>

I will google a little bit what is the "entity_test" entity type and how can I use it.

I will google a little bit what is the "entity_test" entity type and how can I use it.

It's an entity commonly used in our tests. Like a node, but not a node ;)

A special page callback is necessary, because we have to call the $entity->save() method right after the entity_view(), to make the changes permanent what the field formatters does.

That's not right - you can see my example in #5, it's enough to load the entity, and call entity_view() yourself. In fact you don't even have to save it to see the problem. Right after the entity_view() you should see that the same values on the field are the same ($entity->field_foo->getValue() should be the same).

Assigned:Unassigned» Amitaibu

I'm going to work on the test, as it holds other issues I'm working on.

Status:Needs work» Needs review
StatusFileSize
new7.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,616 pass(es).
[ View ]

Patch has a test for all formatters.

StatusFileSize
new7.71 KB
PASSED: [[SimpleTest]]: [MySQL] 59,024 pass(es).
[ View ]

Little cleanup. btw, no interdiff from #12 as the test is different and simplified.

Status:Needs review» Reviewed & tested by the community

#18 looks great.

StatusFileSize
new902 bytes
new7.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,062 pass(es).
[ View ]

Minor fix of a comment.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFormatterTest.php
@@ -0,0 +1,95 @@
+    $formatter_manager = \Drupal::service('plugin.manager.field.formatter');

Shouldn't this use $this->container->get()?

Priority:Critical» Normal
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs followup

Committed #20 to 8.x. Needs follow-up for the comment based in #21.

Can we also open a follow up for discussing whether we're okay with making it every formatter's (including those in contrib) responsibility to check access, and if there's any ideas on how to centralize that?

Title:Access protected field items being removedFollow-up: Access protected field items being removed

Title:Follow-up: Access protected field items being removedAccess protected field items being removed
Priority:Normal» Critical
Status:Needs work» Fixed

Restoring status -- there are two separate followups so let's file them separately.

Title:Access protected field items being removedFollow-up: Access protected field items being removed
Status:Fixed» Active

OK, but let's close this when they're opened.

> Committed #20 to 8.x.

Was this indeed committed? I can't seem to find the commit/ file.

Status:Active» Needs review
StatusFileSize
new966 bytes
new7.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,197 pass(es).
[ View ]

I'm pretty sure this patch wasn't committed - I can't find it in ``git log``

Patch address comment on #21

Title:Follow-up: Access protected field items being removedAccess protected field items being removed

Not sure what's the second follow-up, $this->container->get()? If so, opened #2089337: Decide if tests are allowed to use \Drupal::something() instead of $this->container->get() which is actually the opposite of what you said but I wanted to open that for a while now.

Status:Needs review» Fixed

Committed/pushed (really) to 8.x, thanks!

Status:Fixed» Closed (fixed)

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