FileFormatterBase should extend EntityReferenceFormatterBase

For that, EntityReferenceFormatterBase should be in Core rather than in entity_ref module.
Thus, postponed on #2404021: entity_reference formatters should be in Core

#2404021: entity_reference formatters should be in Core is in.

Initial attempt - this lets us simplify / unify quite some code down all file / image formatters.

Still @todo - would be really nice if this let us get rid of "the default image has a side effect of modifying the $items in the $entity that gets rendered". Mulling on that.

Also, this actually fixes a bug: currently file/image formatters do not follow the correct translation behavior that was added for ER fields in #2346315: Translated entity references not rendered in the entity display language

#2346315: Translated entity references not rendered in the entity display language was major, so I guess this one is too.

#2 will fail on 'default_image'. This should fix it.

Headdesk of the month.

Fails in ToolbarAdminMenuTest look like an unrelated bot fluke, test is green locally

Fails in ImageFieldDefaultImagesTest were because basing on ERFormatterBase means we now check entity access, and that seems to fail for the default image used in that test.

For now, working around this by forcing the "fake" 'access' property in the runtime-added item for the default image. Will need more thought, and #2374019: Cleanup the use of custom item properties in EntityReferenceFormatterBase intends to get rid of that property anyway.

But the fact that file/image formatters currently do not check entity access would make this critical ?

@jibran : Sure, docs still missing, and there are @todos. This is still a WIP :-)

How would DER be affected ?

We can override ERFormatterBase::getEntitiesToView() and manually append the fake $item/$file to the returned array, no?

Yup, that would be the idea. I started that in a local branch based on this patch. Not sure yet if I'll include it in the next iteration or keep it for a followup.

IMO it makes a lot of sense for the "fallback image" : the "entities to view" are derived from the $items, but do not modify the $items.

The issue is that getEntitiesToView() is fine for ER formatters, because an ERItem only contains an entity (a target_id).
But File and Image items contain other stuff ('alt', 'title'...), so the formatters use the entity but also the item that references the entity.

In the current patch, they do :

foreach ($this->getEntitiesToView() as $delta => $entity) {
  $item = $items[$delta];
  // Do stuff with $entity and the $item that refers to it.
}

In the case of the "fallback image", we can add it as an entity in getEntitiesToView(), but the formatter will also need the associated "fake item", that we don't want to add in $items.

In my local branch, I worked around that by having ImageFormatterBase::getEntitiesToView() add $entity->_referringItem in all the returned entities. That way the formatters can call getEntitiesToView() and have both the entity and the item. Not completely ideal, but I can't think of a better solution atm.

And then I'm wondering whether it would make sense to move that $entity->_referringItem thing all the way up in ERFormatterBase::getEntitiesToView().

getItemsToView() : why not, not too sure yet. I considered that option as well, but:

- I wasn't completely a fan of

public function viewElements($items) {
  $items = $this->getItemsToView($items);
  ...
}

:-)

- In relation to #2374019: Cleanup the use of custom item properties in EntityReferenceFormatterBase : if formatters go back to working with $items / $item (those returned by getItemsToView()), we can't expect them to work with $item->originalEntity or whatever other custom property name contains the "right" entity. They should do $this->entity as usual. Hence my last patch in that issue over there, which should make that point a non-issue.

- More importantly : would we also move ERFormatterBase and the ER formatters from getEntitiesToView() to getItemsToView() ?
Feels weird to have ER formatters use getEntitiesToView() and tell field/image formatters "yeah, there is a getEntitiesToView(), but it's not for you, you guys have getItemsToView() instead" ?

@larowlan - good question, that puzzled me too. But this is what HEAD does atm, the patch just moves this code around.

For now, just a reroll after #2374019: Cleanup the use of custom item properties in EntityReferenceFormatterBase.

Shows that the split between prepareView() and getIdsToLoad() in ERFormatterBase is not ideal. Left a @todo, will address in a later patch.

Of course, like mentioned in #10, this fails now that we can't hack the $entity_ref_item->acces property of the "default image" anymore :-p. Will look into fixing access checks in those tests at last.

Puts the iteration back into prepareView(), as mentioned in #23.

This will still fail because of #25 (access check on the fallback image), that's the next target.
Still running the bot to check there were no other regressions.

Work in progress for #15 - #17, here's what I have.
Will post more explanations in a later comment :-)

So, funny things :

- we cannot check entity access on the fallback image (HEAD currently doesn't, but latest patch does), since core's FileAccessControlHandler grants access based on whether there exists an entity the user can access that referes to the file. But the principle of the fallback image is that it can totally happen that no entity actually refers to it.

- given the above on how FileAccessControlHandler works, it seems a bit absurd (and costly) to have image/file formatters check access on file/image entities before displaying them ? If we are displaying the field, it means the user should have view access to the parent entity, and having FileAccessControlHandler re-do that there exists such an entity is useless (and also, might break on preview I guess, since the entity that refers to the file might not be created in the db yet ?)

- but the reasoning above is only valid for the current FileAccessControlHandler - what if a different access handler is swapped in for files ?

Yes, I was thinking of something like that too.

Drawback is that we'd hardcode behavior on a specific access handler class. If you swap a similar but just slightly different class (say, a subclass that changes a minor behavior, or changes behavior for grants other than 'view'), you're out of luck.

Maybe the file access handlers should have a method for "call me in file/image formatters" ? :-/

Curious to know what @fago and @Berdir think about it, but this looks fine by me.

Will see how to integrate that behavior in the generic ERFormatterBase::getEntitiesToView()

Yup, better.

Done as per #33 and #37 :
FileFormatterBase only checks access if the file access control handler implements FileAccessFormatterControlHandlerInterface, which the default file handler does not.

I tried to add understandable explanations in that newly added FileAccessFormatterControlHandlerInterface, suggestions welcome on how to make it less verbose - or how to phrase a first line that makes sense *and* doesn't go over 80 chars :-p

This seems to make the image tests pass again, let's see if this broke something else.

(patch also adjusts a couple vars names, and uses the _referringItem pattern consistenly in all file / image formatters - more on that when we're green)

@googletorp: The patch is still in progress, I was waiting for it to settle before filling in the docs. Thanks for stepping in - when uploading a new patch on an issue, it is really recommended to post an interdiff alongside, so that people can check you changes, and the patch author can integrate them in its local branch :-)

Will process @jibran's remarks in a later post.

@mikey_p: yep, I added it back (manually added @googletorp's changes on top of my own branch)

This attempts to fix ContentEntityBase::__set() messing with TypedData objects added as non-field properties. Will post that as a separate patch if green.

Also adjusts the docs added in #42 a bit.

@googletorp, @jibran : That change is correct in itself, but is not related to this task here, this is existing code that the patch just moves around. Plus, ImageFormatter has the same code, and should be updated as well..

--> I'll revert #46 in the next patch I roll, please open a separate issue for this.

@jibran:

+  protected function needsAccessCheck(EntityReferenceItem $item) {

Can we use interface here? Else i have to override this for DER.

Problem is, we don't have an EntityReferenceItemInterface atm. Arguably we should, since EntityReferenceItem adds a specific public hasNewEntity() method. Care to open an issue for that ?

Opened #2426509: ContentEntityBase::__set() messes with values that happen to be TypedData for the bug in ContentEntityBase::__set() that prevents $entity->_referringItem = $item.
Patch here still contains the fix for now, added a link.

Reverts #46, interdiff is with #45.

Thanks @Wim, will address those asap. Also, I still need to explain a bit why I went with the $entity->_referringItem approach, but it's too late tonight.

Note that this is blocked on #2426509: ContentEntityBase::__set() messes with values that happen to be TypedData though.

re @Wim #50 :

1. Fixed
2. :-)
3. I don't get this point, the sentence looks correct to me, and the proposed replacement doesn't :-)
"[that check] might be needed if a different access control handler with (= that has a) different logic is swapped in", no ?
4. Lol, "implemint" sounds delicious, but well, fixed :-p
5. Because we're checking "is $class_name as subclass of $other_class_name", instanceof doesn't work for that

Enhanced docs a bit.

So, about the $entity->_referringtems approach :

Sorry for making this such a mistery, just had a couple busy days with little quality time in front of my keyboard. Well, also took me a little while to actually remember the reason :-p, but it's actually pretty simple :

getEntitiesToView() is about providing an API to ERFormatter::viewElements() implementations for "just give me the entities I should display, *and* the corresponding items that reference them". That API takes care of checking access and putting the entities in the right language, so that it is not left for each formatter to figure out (with large chances of implementing it wrong or not at all).

Now, if that API primarily returned field items instead of entities (i.e if it was getItemsToView() instead of getEntitiesToView()):
- the formatters implementations would access the referenced entity with the regular "$item->entity", which doesn't give you the entity in the correct language (and we don't want to modify $item->entity to contain the entity in the right language, because we don't want to modify the containing entity being rendered - viewing an entity shouldn't modify that entity as a side effect)
- filtering out items for access check means we'd have to clone the FieldItemList (again, we don't want to modify the containing entity being displayed), which would be a slight perf impact.

So, we stick to returning an array of entities for display, and on each entity you can access the item that referenced it - that's $entity->_referringItem :-)

Side note : an alternative could be to use SplObjectStorage to return an "array" with FieldItem object as keys and Entity objects as values, letting consumers do foreach ($this->getEntitiesToView() as $item => $entity)

Not sure it's worth the extra CPU cost - also, we don't use SplObjectStorage that much, it could look unfamiliar / surprising.

Reading http://technosophos.com/2009/05/29/set-objects-php-arrays-vs-splobjectst..., it seems there is no perf impact regarding SplObjectStorage - they're even supposed to be faster :-). Given our use case here (most likely under 100 items), it would probably not make a difference anyway.

The difference betwen the two approaches is then:
- exposing SplObjectStorage in a relatively high-level API (entity ref formatters), not really a typical pattern in Drupal so far
- the SplObjectStorage approach loses track of the delta, you need to get it from the $item, using the (totally unintuitive) getName() method from TypedData (which, for FieldItems in a FieldItemList, happens to be the delta) :

// Current approach:
foreach ($this->getEntitiestoView() as $delta => $entity) {
  $item = $entity->_referringItem;
  $element[$delta] = [... $entity / $item ... ];
}

// SplObjectStorage approach:
foreach ($this->getEntitiestoView() as $item => $entity) {
  $delta = $item->getName();
  $element[$delta] = [... $entity / $item ... ];
}

This kind of sucks, because not all formatters actually need the $item, but they all need the $delta :-)

So yeah, I can't say I'm married to it, but I'm still inclined towards the _referringItem approach at this point.

@alexpott:

Plus we're obviously not running access checks on files still then - which the summary claims this fixes

Yes, this was decided in #32 / #35 :
- it actually makes no sense and would just add useless overhead to check file access with core's default FileAccessControlHandler, given its internal logic
- *but*, if a different access handler with different logic was switched in, then not running file access would be a security hole.
Thus, what the patch fixes is the ability for the file access controller to opt-in for "have file formatters run me" if their logic requires it.

(added that to the IS)

Will need to think of how to test this with a dummy file access handler class.

Also - we would likely need to move TaxonomyFormatterBase to EntityReferenceFormatterBase as well :-/.

It looks like it currently handles translation and access checks correctly, so that wouldn't be a bug fix, just a maintainability enhancement by avoiding one-off duplicate logic. Opened #2433513: [PP-1] TaxonomyFormatterBase should extend EntityReferenceFormatterBase

@amateescu : for the custom access handler to be responsible for anything, it needs to be called in the first place ;-)

The logic we're facing is :
- We don't want FileFormatters to run the default file access controller (FileAccessControlHandler), because for the specific logic in FileAccessControlHandler that would super costly and useless anyway
- But we want FileFormatters to be able to call the file access controller if it's *not* FileAccessControlHandler

So:
- It's the responsibility of the file access controller to specify that it wants to be called by FileFormatters (by implementing FileAccessFormatterControlHandlerInterface)
- It's the responsibility of FileFormatters to comply and actually call the access controller if it did the above :-)

Thanks a lot @larowlan ! Sorry, hard to carve out time in front of my IDE lately :-/

Thus, setting back to RTBC ?