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
CommentFileSizeAuthor
#12 file-formatter-base-2403873.pass_.patch2.16 KBlarowlan
#12 file-formatter.fail_.patch1.24 KBlarowlan
#9 file-formatter-base-2403873.3.patch938 byteslarowlan
#5 file-formatter-base-2403873.2.patch3.97 KBlarowlan
#5 interdiff.txt1.19 KBlarowlan
file-formatter-you-owe-me-2-hrs-of-my-time.patch1.03 KBlarowlan

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#2403793: EntityReferenceItem uses a static, but it was most likely supposed to be a constant
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

introduced in #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Postponed » Needs review

Blocker is in.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
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

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.19 KB
new file-formatter-base-2403873.2.patch3.97 KB

Thanks

Status: Needs review » Needs work

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

yched’s picture

yched commented
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

yched commented

Opened #2404021: entity_reference formatters should be in Core

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
StatusFileSize
new file-formatter-base-2403873.3.patch938 bytes

reroll

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs tests

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: -Needs tests
StatusFileSize
new file-formatter.fail_.patch1.24 KB
new file-formatter-base-2403873.pass_.patch2.16 KB

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Agreed, let's do this.

Opened #2405469: FileFormatterBase should extend EntityReferenceFormatterBase, postponed on #2404021: entity_reference formatters should be in Core

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Thank you

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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

yched commented

Unpostponed #2405469: FileFormatterBase should extend EntityReferenceFormatterBase and posted an initial patch.

Status: Fixed » Closed (fixed)

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