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

CommentFileSizeAuthor
#32 inexistent-default-image-2226241-32.patch4.92 KBvoidberg
#19 inexistent-default-image-2226241-19.patch4.94 KBvoidberg
#14 inexistent-default-image-2226241-14.patch4.86 KBvoidberg
#9 inexistent-default-image-2226241-9.patch3.59 KBvoidberg
#3 inexistent-default-image-2226241-3.patch2.67 KBvoidberg
#3 inexistent-default-image-test-2226241-3.patch1.65 KBvoidberg
#1 2226241-1.patch1.65 KBclaudiu.cristea

Comments

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2226241-1.patch1.65 KB

This patch is proving the bug.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
voidberg’s picture

voidberg commented
StatusFileSize
new inexistent-default-image-test-2226241-3.patch1.65 KB
new inexistent-default-image-2226241-3.patch2.67 KB

Reattaching test (that should fail) and a patch + test that should fix the issue.

voidberg’s picture

voidberg commented
Status: Active » Needs review

The last submitted patch, 3: inexistent-default-image-test-2226241-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: inexistent-default-image-2226241-3.patch, failed testing.

The last submitted patch, 1: 2226241-1.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

3: inexistent-default-image-2226241-3.patch queued for re-testing.

voidberg’s picture

voidberg commented
StatusFileSize
new inexistent-default-image-2226241-9.patch3.59 KB

Better handling of default values, fix for the issue and test.

Status: Needs review » Needs work

The last submitted patch, 9: inexistent-default-image-2226241-9.patch, failed testing.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review
voidberg’s picture

voidberg commented

9: inexistent-default-image-2226241-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: inexistent-default-image-2226241-9.patch, failed testing.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review
StatusFileSize
new inexistent-default-image-2226241-14.patch4.86 KB

Original test, better handling of default values, fix for the issue, fix for user tests.

Status: Needs review » Needs work

The last submitted patch, 14: inexistent-default-image-2226241-14.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented 
+++ b/core/modules/image/image.module
@@ -403,20 +405,19 @@ function image_entity_presave(EntityInterface $entity) {
-  else {
-    $entity->settings['default_image'] = array(
...
-      'alt' => '',
-      'title' => '',
-      'width' => NULL,
-      'height' => NULL,
-    );

The error shows that settings are not correctly initialized. Maybe we should keep here the else statement and set settings to array()?

else {
  $entity->settings['default_image'] = array();
}

It seems to me that the array merge, 2 lines bellow, fails because the first element is not an array (?)

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

#16 is right, but we can further simplify by:

+++ b/core/modules/image/image.module
@@ -403,20 +405,19 @@ function image_entity_presave(EntityInterface $entity) {
+  $entity->settings['default_image'] += $default_settings['default_image'];
 }

Move this line of code to above the if statement and make it an assignment. Then, subsequently, entries are overwritten when available:

+  $entity->settings['default_image'] = $default_settings['default_image'];
voidberg’s picture

voidberg commented

Neither #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 the Undefined 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 in UserCreateTest.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review
StatusFileSize
new inexistent-default-image-2226241-19.patch4.94 KB

Attaching new patch.

Status: Needs review » Needs work

The last submitted patch, 19: inexistent-default-image-2226241-19.patch, failed testing.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

Well, 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.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented 
+++ b/core/modules/image/image.module
@@ -403,20 +405,19 @@ function image_entity_presave(EntityInterface $entity) {
-  else {
...
-    );
-  }

Keep the else { } block but inside just add:

EDITED:

$entity->settings['default_image'] = $default_settings['default_image'];

This should work. Hopefully :)

voidberg’s picture

voidberg commented
Status: Needs work » Needs review

19: inexistent-default-image-2226241-19.patch queued for re-testing.

voidberg’s picture

voidberg commented

fietserwin, what are the cases when $default_settings['default_image'] is uninitialized?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

Thank you for the patch. Looks nice. Let's move on.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

My 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.

voidberg’s picture

voidberg commented

Yes, it's always initialized.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: inexistent-default-image-2226241-19.patch, failed testing.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review

19: inexistent-default-image-2226241-19.patch queued for re-testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC then.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDefaultImagesTest.php
@@ -289,4 +289,43 @@ public function testDefaultImages() {
+          'fid' => mt_rand(100, 999999),
...
+          'fid' => mt_rand(100, 999999),

Why use random here? It is not adding anything to the test accept the possibility of a random fail at some point.

voidberg’s picture

voidberg commented
StatusFileSize
new inexistent-default-image-2226241-32.patch4.92 KB

Replaced the random number with a fixed value.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

An interdiff file would be very helpful.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Reviewed & tested by the community

Quickly tabbing between the 2 patches gives you a good visual interdiff :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed f61b049 and pushed to 8.x. Thanks!

  • Commit f61b049 on 8.x by alexpott: 
    Issue #2226241 by voidberg, claudiu.cristea: Saving image field or...

Status: Fixed » Closed (fixed)

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