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.
Comment | File | Size | Author |
---|---|---|---|
#70 | Screen Shot 2015-06-15 at 3.03.06 PM.png | 76.42 KB | wOOge |
#61 | drupal-751168-61-combined.patch | 14.2 KB | tim.plunkett |
#61 | drupal-751168-61-tests.patch | 9.67 KB | tim.plunkett |
#56 | 751168-tests.patch | 9.69 KB | xjm |
#56 | 751168-complete.patch | 14.25 KB | xjm |
Comments
Comment #1
elliotttf CreditAttribution: elliotttf commentedHere'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.
Comment #3
elliotttf CreditAttribution: elliotttf commentedWhoops, didn't diff from head. Re-rolled.
Comment #4
elliotttf CreditAttribution: elliotttf commentedComment #5
sunIf 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)
We should encapsulate both conditions into one outer empty() condition.
78 critical left. Go review some!
Comment #6
elliotttf CreditAttribution: elliotttf commentedTotally 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.
Comment #8
elliotttf CreditAttribution: elliotttf commented#6: image-751168.patch queued for re-testing.
Comment #10
elliotttf CreditAttribution: elliotttf commentedHm, 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.
Comment #11
elliotttf CreditAttribution: elliotttf commentedOk, 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.
Comment #12
sunAFAICS, field instance settings are separated from field settings, so we should use 'default_image' here, too.
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!
Comment #13
elliotttf CreditAttribution: elliotttf commentedYeah, 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.
Comment #14
sunExtra parenthesis around ($file = file_load($fid)) is required here.
78 critical left. Go review some!
Comment #15
elliotttf CreditAttribution: elliotttf commentedOk, changed.
Comment #16
sunSorry, lost track of this issue. Looks good, let's get it in to restore D6 functionality.
Comment #17
sunwow, this is still not in...? WTF?
Comment #18
sun#15: image-751168.patch queued for re-testing.
Comment #19
tom_o_t CreditAttribution: tom_o_t commented#15: image-751168.patch queued for re-testing.
Comment #20
sunAs mentioned in the OP, this is a clear regression compared to D6.
Comment #21
webchickThough 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.
Comment #22
Alexander Matveev CreditAttribution: Alexander Matveev commentedsubscribe
Comment #24
sunoh my.
Comment #25
catch#15: image-751168.patch queued for re-testing.
Comment #27
Bojhan CreditAttribution: Bojhan commentedComment #28
sunThis is anything but quick or novice. The increasing amount of noise on issues frustrates me.
Re-rolled against latest branch head.
Still needs tests.
Comment #29
loganfsmyth CreditAttribution: loganfsmyth commentedThis looks good to me.
Comment #30
catchWe still need tests here. Patch looks fine apart from that though.
Comment #31
caktux CreditAttribution: caktux commentedTested 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...
Comment #32
caktux CreditAttribution: caktux commentedHere's the same patch for D7
Comment #33
wojtha CreditAttribution: wojtha commentedComment #35
andyceo CreditAttribution: andyceo commentedStart 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:
Content Look and Feel:
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.
Comment #36
andyceo CreditAttribution: andyceo commented#35: drupal-default_image-751168-35.patch queued for re-testing.
Comment #38
andypost#35: drupal-default_image-751168-35.patch queued for re-testing.
Comment #40
andyceo CreditAttribution: andyceo commentedreroll patch
Comment #41
andyceo CreditAttribution: andyceo commentedComment #42
andyceo CreditAttribution: andyceo commentedtests still not added
Comment #43
setvik CreditAttribution: setvik commentedAdded tests and rerolled patch in #40
Comment #44
xjmI 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!
Comment #45
setvik CreditAttribution: setvik commentedFixed whitespace issues, tidied up inline comments, and added a test-only patch with help from @xjm.
Comment #46
xjmLooks 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.
Comment #47
xjmAlright this turned out to be a little more work than I thought. Attached:
t()
from the assertion messages and usesformat_string()
where appropriate. (Reference: http://drupal.org/simpletest-tutorial-drupal7#t)No interdiff because I reformatted most of the patch.
Comment #51
xjmTypos in that patch, plus I noticed two other small things:
This hunk could probably use some inline comments.
I just noticed the indentation is off here.
Fixing these things.
Comment #56
xjmGrrr. Sorry for the noise.
Comment #57
xjmAs 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().)
Comment #58
tim.plunkettDespite 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.
Comment #59
sunwow, this turned into quite a patch. :) Thanks!
Comment #60
catchYeah 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.
Comment #61
tim.plunkettPatch applied with minor offset, let's see what the bot says.
Comment #63
tim.plunkettAwesome!
Comment #64
xjmString 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.
Comment #65
webchickI 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.
Comment #66
frankcarey CreditAttribution: frankcarey commentedAfter 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?
Comment #67
tim.plunkettThat was fixed in #1558548-4: Notice: Undefined index: default_image in image_field_prepare_view(), setting this issue back.
Comment #69
MiniEggs CreditAttribution: MiniEggs commentedWas this still not fixed? Setting a default image on one of my content types seems to apply it to all of them.
Comment #70
wOOge CreditAttribution: wOOge commentedThis 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?
Comment #71
wOOge CreditAttribution: wOOge commentedComment #72
simohell CreditAttribution: simohell commentedThere are 2 places to set the default image. 1 for single field instance, 1 for every instance.
Comment #73
simohell CreditAttribution: simohell commentedComment #74
wOOge CreditAttribution: wOOge commented@simohell — yes you are correct. my bad. Thank you!
Comment #75
xjm