I just happened to compare the field configuration form between D6 and D7, and while I saw many nice improvements, there's one major flaw and regression:

You can specify (upload) a default image file, but that default image file "applies to the Image field everywhere it is used."

huh? This makes no sense. I can perfectly re-use my field_image in various bundles, perhaps even across different entity types. But it's very unlikely that the same default image can be used everywhere.

The default image file information only seems to be stored in {field_config}.data. There is also a {field_config_instance}.data. So moving this info should be easy.

I've skimmed some other issues related to field default values, but those are targeting default values that actually store data (e.g. text fields). In the case of a file or image field, if no file is uploaded, we should also store no data, and only overload the default value if there is no data (IMO).

If we really happen to store a file id reference to the default file, then be it (makes little sense to me though). We'll still do that, but the default image file id should come from the instance, not the field.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

elliotttf’s picture

Status: Active » Needs review
FileSize
1.88 KB

Here's a patch that allows a per-instance default file in addition to the existing per-field default file. I agree that having per-instance default files is desirable but I also think having the per-field default is still desirable too in case you wanted a more general default file to be used throughout the site.

Status: Needs review » Needs work

The last submitted patch, image-751168.patch, failed testing.

elliotttf’s picture

FileSize
2 KB

Whoops, didn't diff from head. Re-rolled.

elliotttf’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Needs work
+++ modules/image/image.field.inc	13 May 2010 04:44:48 -0000
@@ -174,6 +175,14 @@ function image_field_instance_settings_f
+    '#description' => t('If no image is uploaded, this image will be shown on display.'),

If we really want to keep the inheritance from instance default to field default, then this description probably needs to be updated.

Not sure whether I agree with the inheritance though (not used anywhere else)

+++ modules/image/image.field.inc	13 May 2010 04:44:48 -0000
@@ -213,7 +222,16 @@ function image_field_load($entity_type, 
-    if (empty($items[$id]) && $field['settings']['default_image']) {
+    if (empty($items[$id]) && $instances[$id]['settings']['instance_default_image']) {
+      if ($file = file_load($instances[$id]['settings']['instance_default_image'])) {
+        $items[$id][0] = (array) $file + array(
+          'is_default' => TRUE,
+          'alt' => '',
+          'title' => '',
+        );
+      }
+    }
+    else if (empty($items[$id]) && $field['settings']['default_image']) {

We should encapsulate both conditions into one outer empty() condition.

78 critical left. Go review some!

elliotttf’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

Totally agree about the message. I've rerolled the patch with a different message: "If no image is uploaded, this image will be shown on display and will override the field's default image." and refactored the default check as requested. I think there could still be some improvement for the message but I'm not sure exactly how to word it.

As for inheritance, I think in this instance it's a desirable feature. It's possible that one image should be used as the default for most instances but still be selectively overridden for others. In this case it would be easier (and desirable in my opinion) to upload the global default once and then only have to upload again if you need to override the default.

Status: Needs review » Needs work
Issue tags: -API change

The last submitted patch, image-751168.patch, failed testing.

elliotttf’s picture

Status: Needs work » Needs review

#6: image-751168.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change

The last submitted patch, image-751168.patch, failed testing.

elliotttf’s picture

Hm, not sure why the patch failed testing. I did notice that (even without the patch) the default file status is not being set to 1 and would presumably be wiped out by cron eventually. That's the only thing I could think of though since most of the warnings are complaining about nonexistent files.

elliotttf’s picture

Ok, it looks like the file status issue is covered here: #618654: File and image fields are treated as temporary files and automatically deleted after six hours, still not sure why the patch passed before refactoring but failed horribly after though.

sun’s picture

+++ modules/image/image.field.inc	17 May 2010 23:01:09 -0000
@@ -26,6 +26,7 @@ function image_field_info() {
+        'instance_default_image' => 0,

@@ -174,6 +175,14 @@ function image_field_instance_settings_f
+  $form['instance_default_image'] = array(

AFAICS, field instance settings are separated from field settings, so we should use 'default_image' here, too.

+++ modules/image/image.field.inc	17 May 2010 23:01:09 -0000
@@ -213,14 +222,22 @@ function image_field_load($entity_type, 
+      if ($instances[$id]['settings']['instance_default_image']) {
+        $file = file_load($instances[$id]['settings']['instance_default_image']);
+      }
+      else if ($field['settings']['default_image']) {
+        $file = file_load($field['settings']['default_image']);
+      }
+
+      if ($file) {
         $items[$id][0] = (array) $file + array(
...
       }
+      $file = NULL;

1) $fid = 0; should be reset at the beginning. (instead of the end/NULL)

2) The conditions should assign the $fid value only.

3) "elseif" instead of "else if" (no space).

4) The final condition should be if ($fid && ($file = file_load($fid)))

81 critical left. Go review some!

elliotttf’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

Yeah, I had noticed that the fields were separate originally and considered naming them the same initially but decided to make it different just so it was obvious in the code which element was being called. If you're not worried about it though I won't be either :).

Rerolled the patch with the most recently requested changes.

sun’s picture

Status: Needs review » Needs work
+++ modules/image/image.field.inc	18 May 2010 18:17:32 -0000
@@ -213,8 +222,16 @@ function image_field_load($entity_type, 
+      if ($fid && $file = file_load($fid)) {

Extra parenthesis around ($file = file_load($fid)) is required here.

78 critical left. Go review some!

elliotttf’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

Ok, changed.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, lost track of this issue. Looks good, let's get it in to restore D6 functionality.

sun’s picture

wow, this is still not in...? WTF?

sun’s picture

Issue tags: -API change

#15: image-751168.patch queued for re-testing.

tom_o_t’s picture

Issue tags: +API change

#15: image-751168.patch queued for re-testing.

sun’s picture

Title: "Default image" per field (not per instance) makes no sense » Regression: Missing "Default image" per field instance
Priority: Normal » Major

As mentioned in the OP, this is a clear regression compared to D6.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Though technically this breaks UX freeze, agreed that this is a pretty silly regression from D6 and we should fix it. However, we need tests for this.

Alexander Matveev’s picture

subscribe

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

oh my.

catch’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -API change, -Needs backport to D7

#15: image-751168.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +API change, +Needs backport to D7

The last submitted patch, image-751168.patch, failed testing.

Bojhan’s picture

Issue tags: +Quick fix
sun’s picture

Status: Needs work » Needs review
Issue tags: -Quick fix +Regression
FileSize
1.79 KB

This is anything but quick or novice. The increasing amount of noise on issues frustrates me.

Re-rolled against latest branch head.

Still needs tests.

loganfsmyth’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

catch’s picture

Status: Reviewed & tested by the community » Needs work

We still need tests here. Patch looks fine apart from that though.

caktux’s picture

Tested the patch in #28 on D7 but the image was getting deleted after a while, just like #1028092: Default image is not set to permanent and saved to the wrong schema. Here's a patch with the same solution. Still needs the test...

caktux’s picture

Here's the same patch for D7

wojtha’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, d7-image-instance-default.patch, failed testing.

andyceo’s picture

Status: Needs work » Needs review
FileSize
8.26 KB

Start writing tests, just for 8.x now, when it be ready, I`ll backport it to the 7.x.

Assertion plan is:
Field Instances Admin Forms:

  1. look at article field instance admin form and assert for field default and instance default image (should be different)
  2. do the same with page field instance admin form (but field default from article should be equal to field default from page form, all other image defaults should be different)

Content Look and Feel:

  1. create article without image (should be default of article field instance)
  2. create page without image (should be default of page field instance)
  3. upload new default for articles instance (should be new default on created article, old default on created page)
  4. upload new default for field (nothing should be changed)
  5. remove instance default from article (should be new field default on created article, old default on created page)

Please give me feedback, am I doing it right or something need to be change. I`ll continue in a few days or after feedback.

andyceo’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-default_image-751168-35.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Regression, +API change, +Needs backport to D7

The last submitted patch, drupal-default_image-751168-35.patch, failed testing.

andyceo’s picture

reroll patch

andyceo’s picture

Status: Needs work » Needs review
andyceo’s picture

Status: Needs review » Needs work

tests still not added

setvik’s picture

Status: Needs work » Needs review
FileSize
13.57 KB

Added tests and rerolled patch in #40

xjm’s picture

I talked to @setvik in IRC -- setvik will upload a test-only patch, plus clean up the comments and whitespace a bit in this patch. Awesome work!

setvik’s picture

Fixed whitespace issues, tidied up inline comments, and added a test-only patch with help from @xjm.

xjm’s picture

Assigned: Unassigned » xjm
Issue tags: -Needs tests

Looks good to me. Thanks @setvik! The test covers the assertions outlined in #35, and the failures without the fix are exposed in the test-only patch in #45.

Given the number of assertions we are making, I think it might be better to use multiline format for readability. I'll reroll @setvik's patch with those changes and maybe tweak the docs a little more.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
14.17 KB

Alright this turned out to be a little more work than I thought. Attached:

  • Refactors some dense ternaries in the original fix for better readability.
  • Improves the inline documentation in the original fix.
  • Reformats the assertion messages for better readability.
  • Replaces "FID" with "file ID" in the assertion messages per our text standards.
  • Adds articles to the inline comments to make them more grammatically complete and less terse.
  • Removes t() from the assertion messages and uses format_string() where appropriate. (Reference: http://drupal.org/simpletest-tutorial-drupal7#t)

No interdiff because I reformatted most of the patch.

Status: Needs review » Needs work

The last submitted patch, default-image-751168-47.patch, failed testing.

xjm’s picture

Typos in that patch, plus I noticed two other small things:

+++ b/core/modules/image/image.field.incundefined
@@ -191,8 +201,16 @@ function image_field_load($entity_type, $entities, $field, $instances, $langcode
+    if (empty($items[$id])) {
+      $fid = 0;
+      if ($instances[$id]['settings']['default_image']) {
+        $fid = $instances[$id]['settings']['default_image'];
+      }
+      elseif ($field['settings']['default_image']) {
+        $fid = $field['settings']['default_image'];
+      }
+
+      if ($fid && ($file = file_load($fid))) {

This hunk could probably use some inline comments.

+++ b/core/modules/image/image.moduleundefined
@@ -411,6 +411,73 @@ function image_field_update_field($field, $prior_field, $has_data) {
+ /**
+ * Implements hook_field_delete_instance().
+ */

I just noticed the indentation is off here.

Fixing these things.

xjm’s picture

Status: Needs work » Needs review
FileSize
14.25 KB
9.69 KB

Grrr. Sorry for the noise.

xjm’s picture

As timplunkett pointed out in IRC, the fid of 0 is magical and the structure of the instance array (that sometimes has an fid key and sometimes doesn't) has a distinct code smell. However, both these things exist already in core and are outside the patch scope. (See, for example, image_field_update_field() and image_field_info().)

tim.plunkett’s picture

Despite my reservations that xjm pointed out in #57, this is fine.
The tests look thorough and the fix makes sense.

Leaving for another pair of eyes, but I think this is ready.

sun’s picture

Status: Needs review » Reviewed & tested by the community

wow, this turned into quite a patch. :) Thanks!

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yeah the field instance structure like that is odd and it's a shame it affects the new functions added here, I opened #1544118: Image field instance array structure and magic numbering to track it, but agree it's out of scope here.

Committed/pushed to 8.x, moving to 7.x for backport.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
9.67 KB
14.2 KB

Patch applied with minor offset, let's see what the bot says.

Status: Needs review » Needs work

The last submitted patch, drupal-751168-61-tests.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Awesome!

xjm’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/modules/image/image.field.incundefined
@@ -152,6 +153,15 @@ function image_field_instance_settings_form($field, $instance) {
+    '#description' => t("If no image is uploaded, this image will be shown on display and will override the field's default image."),

String addition here, and there's a UI change in that this is now available on the instance configuration, but I think that's OK for backport.

Same patch, more D7 flavor.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.13 release notes

I think the string addition is acceptable; this is an admin-only facing page, and only shows up on the second part of the form so you have to be pretty deep in the weeds to see it. I'm not sure why this is tagged "API change" though; it seems ok to me. GREAT work on the tests here; extremely thorough.

Committed and pushed to 7.x. Thanks! Tagging for release notes.

frankcarey’s picture

Status: Fixed » Active

After upgrading to 7.14 from 7.12, our Jenkins is throwing this notice:

.image_field_update_instance() FAIL

Undefined index: default_image
line: 501
file: /var/web/aws-d7/drupal/modules/image/image.module

I can look to debug later, but best guess is this is related to the refactoring/new tests?

tim.plunkett’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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

MiniEggs’s picture

Was this still not fixed? Setting a default image on one of my content types seems to apply it to all of them.

wOOge’s picture

Issue summary: View changes
FileSize
76.42 KB

This does not seem to be fixed. See attached screenshot — seems that the default image affects all instances of the same field. Should this not operate like a text field in that each instance in a content type should have it's own default value?

screenshot

wOOge’s picture

Status: Closed (fixed) » Needs work
simohell’s picture

There are 2 places to set the default image. 1 for single field instance, 1 for every instance.

simohell’s picture

Status: Needs work » Fixed
wOOge’s picture

@simohell — yes you are correct. my bad. Thank you!

xjm’s picture

Status: Fixed » Closed (fixed)