Problem/Motivation

FileFormatterBase::prepareView has code to check that the file exists for a given target when a target_id is set, but it does not consider that the target_id could be for an unsaved entity - and in doing so wipes out any unsaved entities.

Proposed resolution

Make it check for unsaved items first.

Remaining tasks

Decide if this is worth adding a test

User interface changes

None

API changes

None

Postponed on #2403793: EntityReferenceItem uses a static, but it was most likely supposed to be a constant because that is required before you can actually use the constant.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because data-loss
Issue priority Major because data loss
Disruption None
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan’s picture

larowlan’s picture

Status: Postponed » Needs review

Blocker is in.

plach’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileFormatterBase.php
@@ -35,7 +36,7 @@ public function prepareView(array $entities_items) {
+          if (!empty($item->target_id) && $item->target_id != EntityReferenceItem::NEW_ENTITY_MARKER) {

There's a convenient $item->hasNewEntity() method for that :)

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
3.97 KB

Thanks

Status: Needs review » Needs work

The last submitted patch, 5: file-formatter-base-2403873.2.patch, failed testing.

yched’s picture

Issue tags: +Entity Field API

The logic provided by FileFormatterBase is now implemented, in a more solid way, by EntityReferenceFormatterBase.

I think we should get rid of FileFormatterBase, and have the existing file / image formatters extend EntityReferenceFormatterBase.
EntityReferenceFormatterBase is currently provided by entity_reference.module, but I think that's wrong, it should be in Core (like the ER formatters and widgets themselves, BTW)

yched’s picture

larowlan’s picture

reroll

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

Issue tags: +Needs tests

Trying to think of how to test this, @alexpott said needs a test

larowlan’s picture

The last submitted patch, 12: file-formatter.fail_.patch, failed testing.

larowlan’s picture

Can we move forward here without waiting for the other issue?
Simple patch with a test here.

yched’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

Thank you

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed fc3381e on 8.0.x
    Issue #2403873 by larowlan: FileFormatterBase does not retain unsaved...
yched’s picture

Status: Fixed » Closed (fixed)

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