Problem/Motivation

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because file field default values cannot be deployed, and deploying a reference to a serial ID that may or may not exist or be the same entity (as in HEAD) can result in errors and unexpected behavior.
Issue priority Not critical because default file field values are not a sufficient reason to hold up the release. Major because the bug breaks the principles of the configuration system, makes file field deployments unreliable, and could introduce data integrity problems for entities having a file field with a default value.
Prioritized changes The main goal of this issue is fixing a bug, so it is a prioritized change for the beta phase.
Disruption Minor disruption for existing sites because it changes the data model for existing file fields (and therefore tagged 'D8 upgrade path').

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#17 interdiff-2357801-14-17.txt708 bytesamitgoyal
#17 2357801-17.patch25.23 KBamitgoyal
#14 2357801-14.patch25.23 KBdawehner
#14 interdiff.txt4.16 KBdawehner
#12 2357801-12.patch21.07 KBdawehner
#12 interdiff.txt2.05 KBdawehner
#9 2357801-9.patch19.01 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue tags: +Configuration system
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Sure we solved this for term reference defaults already so perhaps could adapt that? But regardless - love it.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Use uuid in file fields to point to the default file » Make file field default values deployable by using UUIDs for the default file
Category: Task » Bug report
Priority: Major » Critical
Parent issue: » #2248369: [meta] Deploying configuration that depends on content
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +D8 upgrade path
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Critical » Major

I guess filefield default values being broken on deployment would not hold up release. (The views issue is definitely critical though.)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Make file field default values deployable by using UUIDs for the default file » File field default values are not deployable -- use UUIDs for the default file
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
2357801-9.patch19.01 KB

Worked a bit on it.

Status: Needs review » Needs work

The last submitted patch, 9: 2357801-9.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

If this is anywhere near RTBC by the time we get to 0 upgrade path blockers, this is the kind of issue I'd rather see in before rather than after we start supporting 8-8 updates (so that we don't have to write a hook_update_N() here).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.05 KB
2357801-12.patch21.07 KB

Maybe this is enough already.

Status: Needs review » Needs work

The last submitted patch, 12: 2357801-12.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.16 KB
2357801-14.patch25.23 KB

Let's fix them.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Looks awesome so RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/image/image.module
@@ -340,18 +340,18 @@ function image_entity_presave(EntityInterface $entity) {
+    $original_fid = isset($entity->original) ? $entity->original->settings['default_image']['uuid'] : NULL;

    This should be $original_uuid. Minor but it looked like a logic error at first glance then realised just variable name hadn't been updated. 

  2. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -138,9 +138,13 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    if (!empty($default_image['uuid']) && $entity = \Drupal::entityManager()->loadEntityByUuid('file', $default_image['uuid'])) {

    Won't this be more expensive? Not sure about calculating this in the widget, although could be a follow-up, presumably we have similar issues elsewhere.

amitgoyal’s picture

amitgoyal CreditAttribution: amitgoyal commented
Status: Needs work » Needs review
FileSize
2357801-17.patch25.23 KB
interdiff-2357801-14-17.txt708 bytes

1) Variable name $original_fid has been changed to $original_uuid.
2) May be a follow-up.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Won't this be more expensive? Not sure about calculating this in the widget, although could be a follow-up, presumably we have similar issues elsewhere.

I'd argue that this is "just" for the edit UI, so even requiring the conversion is a bit sad, its not part of the critical path.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Thanks for the variable rename and makes sense on edit UI given it's the widget..

Committed/pushed to 8.0.x, thanks!

  • catch committed 0485e55 on 8.0.x 
    Issue #2357801 by dawehner, amitgoyal: File field default values are not...

Status: Fixed » Closed (fixed)

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