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

Files: 
CommentFileSizeAuthor
#17 interdiff-2357801-14-17.txt708 bytesamitgoyal
#17 2357801-17.patch25.23 KBamitgoyal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,495 pass(es). View
#14 2357801-14.patch25.23 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,827 pass(es). View
#14 interdiff.txt4.16 KBdawehner
#12 2357801-12.patch21.07 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,741 pass(es), 3 fail(s), and 4 exception(s). View
#12 interdiff.txt2.05 KBdawehner

Comments

alexpott’s picture

Issue tags: +Configuration system
larowlan’s picture

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

xjm’s picture

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

Issue tags: +D8 upgrade path
xjm’s picture

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

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

Issue summary: View changes
xjm’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs review
FileSize
19.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,087 pass(es), 15 fail(s), and 87 exception(s). View

Worked a bit on it.

Status: Needs review » Needs work

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

catch’s picture

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

Status: Needs work » Needs review
FileSize
2.05 KB
21.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,741 pass(es), 3 fail(s), and 4 exception(s). View

Maybe this is enough already.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
25.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,827 pass(es). View

Let's fix them.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Looks awesome so RTBC.

catch’s picture

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

Status: Needs work » Needs review
FileSize
25.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,495 pass(es). View
708 bytes

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

dawehner’s picture

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

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.