Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
- Install a new D8 site with standard installation profile
- Enable the entity_reference module
- Add a new entity reference field to node:article - ?q=admin/structure/types/manage/article/fields
- Ensure the field is editable ?q=admin/structure/types/manage/article/form-display
- 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
- Create 3 article content with the following titles (to easily identify them)
- Child 01 published
nid: 1
Leave empty the entity reference field - Child 02 UNpublished
nid: 2
Leave empty the entity reference field - Parent
nid: 3
and refers to the "Child 01" and "Child 02" - Enable the
field_item_lost
module. See below. - Go to ?q=node/3/field-item-lost
- Open an incognito window in your browser and go to Go to ?q=node/3/field-item-lost
- 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.
Comment | File | Size | Author |
---|---|---|---|
#28 | 2078155-er-no-unset-just-check-access-28.patch | 7.7 KB | amitaibu |
#28 | interdiff-28.txt | 966 bytes | amitaibu |
#20 | 2078155-er-no-unset-just-check-access-20.patch | 7.7 KB | amitaibu |
#20 | interdiff-20.txt | 902 bytes | amitaibu |
#18 | 2078155-er-no-unset-just-check-access-17.patch | 7.71 KB | amitaibu |
Comments
Comment #1
SweetchuckI 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?Comment #2
amitaibu> 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()
Comment #3
amitaibuI can confirm the bug in the OP.
Comment #4
amitaibuSeems that it is not related to the formatter. Using your nodes from the OP, I have this inside hook_page_build:
So to reproduce, you don't need the module from the OP, the above lines are enough.
Comment #5
amitaibuSorry, #4 is wrong, I was checking it on the node view itself. Here's the updated code:
Comment #6
amitaibuPatch removes the unsetting of the values, and instead checks
$item->access
. Also renames a wrongly used variable.Comment #7
Sweetchuck@Amitaibu: I would like to practice the test writing, please do not write tests.
Comment #8
amitaibu> @Amitaibu: I would like to practice the test writing, please do not write tests.
Great, thanks!
Comment #9
SweetchuckHere is the patch for test. I start to fix the formatters.
Comment #11
Sweetchuckhttps://qa.drupal.org/pifr/test/613318
Looks good. Only the tree our error occurred.
Comment #12
SweetchuckMerge patches from #6 and #9.
Comment #13
BerdirThanks for working on this. Test is a bit different to what you usually find in core but looks well written and is understandable.
There is no parent implementation it could inherit the docs from, current standard is to leave that out.
This should be above the getInfo() and have a docblock, see other tests.
All properties should be above the methods.
entity_types is no longer necessary.
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.
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.
Same.
We isually don't use the Object[] syntax, to avoid arguments over it, I'd recommend "array".
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.
Comment #14
SweetchuckAbout the hook_menu().
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.
I will google a little bit what is the "entity_test" entity type and how can I use it.
Comment #15
amitaibuIt's an entity commonly used in our tests. Like a node, but not a node ;)
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).
Comment #16
amitaibuI'm going to work on the test, as it holds other issues I'm working on.
Comment #17
amitaibuPatch has a test for all formatters.
Comment #18
amitaibuLittle cleanup. btw, no interdiff from #12 as the test is different and simplified.
Comment #19
catch#18 looks great.
Comment #20
amitaibuMinor fix of a comment.
Comment #21
xjmShouldn't this use
$this->container->get()
?Comment #22
Dries CreditAttribution: Dries commentedCommitted #20 to 8.x. Needs follow-up for the comment based in #21.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedCan 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?
Comment #24
catchComment #25
xjmRestoring status -- there are two separate followups so let's file them separately.
Comment #26
catchOK, but let's close this when they're opened.
Comment #27
amitaibu> Committed #20 to 8.x.
Was this indeed committed? I can't seem to find the commit/ file.
Comment #28
amitaibuI'm pretty sure this patch wasn't committed - I can't find it in ``git log``
Patch address comment on #21
Comment #29
amitaibuFollow up issue -- #2089123: Centralize access check for formatters
Comment #30
BerdirNot 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.
Comment #31
catchCommitted/pushed (really) to 8.x, thanks!