Updated: Comment #0
Problem/Motivation
In #1443606: Alt, title, width and height for default images, we had changed default_image
from integer to an associative array with next keys: fid, alt, title, width, height. For performance reasons the width & height are computed when the field or instance are saved and are stored in field/instance settings.
In some edge cases there is the possibility to save the field/instance with a default image fid of an inexistent image. This will crash with:
Fatal error: Call to a member function getFileUri() on a non-object in core/modules/image/image.module on line 406
I discovered this bug in a simple test, by saving an image field instance with an hypothetical default image.
Proposed resolution
Check the existence of default image file before computing width & height.
Remaining tasks
n/a
User interface changes
n/a
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#32 | inexistent-default-image-2226241-32.patch | 4.92 KB | voidberg |
#19 | inexistent-default-image-2226241-19.patch | 4.94 KB | voidberg |
#14 | inexistent-default-image-2226241-14.patch | 4.86 KB | voidberg |
#9 | inexistent-default-image-2226241-9.patch | 3.59 KB | voidberg |
#3 | inexistent-default-image-2226241-3.patch | 2.67 KB | voidberg |
Comments
Comment #1
claudiu.cristeaThis patch is proving the bug.
Comment #2
claudiu.cristeaComment #3
voidberg CreditAttribution: voidberg commentedReattaching test (that should fail) and a patch + test that should fix the issue.
Comment #4
voidberg CreditAttribution: voidberg commentedComment #8
Berdir3: inexistent-default-image-2226241-3.patch queued for re-testing.
Comment #9
voidberg CreditAttribution: voidberg commentedBetter handling of default values, fix for the issue and test.
Comment #11
voidberg CreditAttribution: voidberg commentedComment #12
voidberg CreditAttribution: voidberg commented9: inexistent-default-image-2226241-9.patch queued for re-testing.
Comment #14
voidberg CreditAttribution: voidberg commentedOriginal test, better handling of default values, fix for the issue, fix for user tests.
Comment #16
claudiu.cristeaThe error shows that settings are not correctly initialized. Maybe we should keep here the
else
statement and set settings to array()?It seems to me that the array merge, 2 lines bellow, fails because the first element is not an array (?)
Comment #17
fietserwin#16 is right, but we can further simplify by:
Move this line of code to above the if statement and make it an assignment. Then, subsequently, entries are overwritten when available:
Comment #18
voidberg CreditAttribution: voidberg commentedNeither #16 or #17 would work because the issue is actually different.
The two tests that failed are creating image fields but are not specifying any option for default_image so the code relies on the default values provided by the
ImageItem's
defaultInstanceSettings
function. This one does not set a proper default value for title causing the default values to not have it, so setting the default values to that will propagate the missing title and cause theUndefined index: title
.I've fixed this as well and this has the nice side effect that we can remove the
default_image
array from the image instances inUserCreateTest
.Comment #19
voidberg CreditAttribution: voidberg commentedAttaching new patch.
Comment #21
fietserwinWell, the fact remains that we are adding a non-initialized variable to an array = bad code = to be changed one way or the other. So remains NW for me, even if the test failures do not seem related.
Comment #22
claudiu.cristeaKeep the
else { }
block but inside just add:EDITED:
This should work. Hopefully :)
Comment #23
voidberg CreditAttribution: voidberg commented19: inexistent-default-image-2226241-19.patch queued for re-testing.
Comment #24
voidberg CreditAttribution: voidberg commentedfietserwin, what are the cases when $default_settings['default_image'] is uninitialized?
Comment #25
claudiu.cristeaThank you for the patch. Looks nice. Let's move on.
Comment #26
fietserwinMy fault, it's the other way around. Is $entity->settings['default_image'] always already an (possibly empty) array before doing the +=? If so, it's indeed good to go.
Comment #27
voidberg CreditAttribution: voidberg commentedYes, it's always initialized.
Comment #29
voidberg CreditAttribution: voidberg commented19: inexistent-default-image-2226241-19.patch queued for re-testing.
Comment #30
claudiu.cristeaBack to RTBC then.
Comment #31
alexpottWhy use random here? It is not adding anything to the test accept the possibility of a random fail at some point.
Comment #32
voidberg CreditAttribution: voidberg commentedReplaced the random number with a fixed value.
Comment #33
voidberg CreditAttribution: voidberg commentedComment #34
claudiu.cristeaAn interdiff file would be very helpful.
Comment #35
fietserwinQuickly tabbing between the 2 patches gives you a good visual interdiff :)
Comment #36
alexpottCommitted f61b049 and pushed to 8.x. Thanks!