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

/**
 * @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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sweetchuck’s picture

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?

amitaibu’s picture

> 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()

amitaibu’s picture

I can confirm the bug in the OP.

amitaibu’s picture

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

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.

amitaibu’s picture

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

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());
}
amitaibu’s picture

Status: Active » Needs review
FileSize
4.71 KB

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

Sweetchuck’s picture

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

amitaibu’s picture

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

Great, thanks!

Sweetchuck’s picture

FileSize
14.7 KB

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.

Sweetchuck’s picture

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

Sweetchuck’s picture

Status: Needs work » Needs review
FileSize
19.41 KB

Merge patches from #6 and #9.

Berdir’s picture

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.

Sweetchuck’s picture

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.

$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.

amitaibu’s picture

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).

amitaibu’s picture

Assigned: Unassigned » amitaibu

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

amitaibu’s picture

Status: Needs work » Needs review
FileSize
7.98 KB

Patch has a test for all formatters.

amitaibu’s picture

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

catch’s picture

Status: Needs review » Reviewed & tested by the community

#18 looks great.

amitaibu’s picture

Minor fix of a comment.

xjm’s picture

+++ 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()?

Dries’s picture

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.

effulgentsia’s picture

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?

catch’s picture

Title: Access protected field items being removed » Follow-up: Access protected field items being removed
xjm’s picture

Title: Follow-up: Access protected field items being removed » Access 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.

catch’s picture

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

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

amitaibu’s picture

> Committed #20 to 8.x.

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

amitaibu’s picture

Status: Active » Needs review
FileSize
966 bytes
7.7 KB

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

Patch address comment on #21

amitaibu’s picture

Berdir’s picture

Title: Follow-up: Access protected field items being removed » Access 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.

catch’s picture

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.