Image field default value not shown when upload destination set to private file storage

This is quite a lot of procedural code, plus the &drupal_static() in there. This looks like D7 code. Is there a reason it has to be added to image.module rather than being better encapsulated?

Tagging for framework manager feedback on the architecture. This could also use @claudiu.cristea's feedback as the subsystem maintainer -- their patch was the original prototype back in 2013, but the codebase has changed quite a lot since then.

1. +++ b/core/modules/image/image.module
   @@ -167,6 +168,115 @@ function image_file_download($uri) {
   +  if (strpos($path, 'default_images/') === 0) {
   

   Can use str_starts_with() here.

2. +++ b/core/modules/image/image.module
   @@ -167,6 +168,115 @@ function image_file_download($uri) {
   +/**
   + * Map default values for image fields, and those fields' configuration IDs.
   + *
   + * This is used in image_file_download() to determine whether to grant access to
   + * an image stored in the private file storage.
   + *
   + * @return array
   + *   An associative array, where the keys are image file URIs, and the values
   + *   are arrays of field configuration IDs which use that image file as their
   + *   default image. For example,
   + *
   + * @code [
   + *     'private://default_images/astronaut.jpg' => [
   + *       'node.article.field_image',
   + *       'user.user.field_portrait',
   + *     ],
   + *   ]
   + * @code
   + */
   +function image_get_default_image_fields() {
   +  $defaults = &drupal_static(__FUNCTION__);
   +  $cid = 'image:default_images';
   +
   +  if (!isset($defaults)) {
   +    if ($cache = \Drupal::cache()->get($cid)) {
   +      $defaults = $cache->data;
   +    }
   +    else {
   +      // Save a map of all default image UUIDs and their corresponding field
   +      // configuration IDs for quick lookup.
   +      $defaults = [];
   +      $fields = \Drupal::entityTypeManager()
   +        ->getStorage('field_config')
   +        ->loadMultiple();
   +
   +      foreach ($fields as $field) {
   +        if ($field->getType() == 'image') {
   +          // Check if there is a default image in the field config.
   +          if ($field_uuid = $field->getSetting('default_image')['uuid']) {
   +            $file = \Drupal::service('entity.repository')
   +              ->loadEntityByUuid('file', $field_uuid);
   +
   +            // A default image could be used by multiple field configs.
   +            $defaults[$file->getFileUri()][] = $field->get('id');
   +          }
   +
   +          // Field storage config can also have a default image.
   +          if ($storage_uuid = $field->getFieldStorageDefinition()->getSetting('default_image')['uuid']) {
   +            $file = \Drupal::service('entity.repository')
   +              ->loadEntityByUuid('file', $storage_uuid);
   +            if ($file) {
   +              // Use the field config id since that is what we'll be using to
   +              // check access in image_file_download().
   +              $defaults[$file->getFileUri()][] = $field->get('id');
   +            }
   +          }
   +        }
   +      }
   +
   +      // Cache the default image list.
   +      \Drupal::cache()
   +        ->set($cid, $defaults, CacheBackendInterface::CACHE_PERMANENT, ['image_default_images']);
   +    }
   +  }
   +
   +  return $defaults;
    }
   

   I agree with @xjm - this code should be a service and the if we do have a cache then it'll need to be cleared when a field is changed.

3. When we add the service we can also move the code from image_file_download there too

Reworks patch #74 for Drupal 9.4.13

Hiding the D9.4 patch; we don't need that. We need #82 and #83 addressed. Thanks!

I just updated to Social (social-11.8.12) and the ugly Default Profile image wouldn't display once again and I couldn't get any of the patches to work until I tried #84 🐛 Image field default value not shown when upload destination set to private file storage Needs work ( 2107455-84.patch → )

At least for now that one solved it in four Drupal Open Social websites! so Kudos to Jan Kellermann → !

I just wish this problem which has persisted for several years and incarnations would ge committed to core...

Just reroll for 10.1.6

Upgrading to core 10.2.1 caused #79 (and #89) to fail to apply.

I was using #79 successfully with 10.1.7.

eiriksm → changed the visibility of the branch 11.x to hidden.

Here is a patch for 10.2

Going to try to take a swing at the feedback in #82 and #83

eiriksm → changed the visibility of the branch 11.x to active.

Opened a MR (against 11.x) where i added back the test module needed to actually run the tests in the patch.

Also refactored the procedural style code into a service.

I'm pretty sure we are missing a bit of test coverage on this one though (for example the code branch concerning "A default image could be used by multiple field configs."), but since it's a bug it might not be the main priority. The test certainly demonstrates the bug at least, so that's the most important part for me.

Anyway. the feedback in #82 and #83 should be addressed in this MR

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Patch #94 via eiriksm worked for me on 10.2.3, many thanks.

Guess it should have status needs review?

Assuming the MR, appears to have test failures.

Patch #94 not working with 10.3, so sticking to 10.2.7 for sites involving this.

@yospyn , are there any watchdog /php log messages or helpful console exceptions that provide some insight?

@yospyn,

The MR was updated, it now rolls with fuzz on 10.3.x, might work for you.

https://git.drupalcode.org/project/drupal/-/merge_requests/6158.diff

Might work with 10.3.x

w00t! All passing as a service now.

Test only fails as expected: https://git.drupalcode.org/issue/drupal-2107455/-/pipelines/210442/test_...

Rationale for RTBC:

• Requested change to a service is completed (see comments #82,#83)
• Merge request is passing tests.
• Tests Only test is failing which proves the need for the fix

@kksandr , I'll leave the other suggested changes for others:

@todo:

• Services should be based on interfaces so they can be decorated or overridden.
• Move this into a new service method image.field_manager something like checkAccessToDefaultImage(string $uri): bool so that it uses dependency injection, and can be overridden by a service override or decoration.
• Check access only after the image is recognized as valid, that is, this should all be under
  

  if ($image->isValid()) {
  

• It could be part of the checkAccessToDefaultImage method which will do all the work regarding the $uri access check.

Reverted 5158f6f6, it didn't seem right to change the storage from field_config to entity_field.manager as suggested by @kksandr, so backing it off. We'll see if this greens it up.

kksandr → changed the visibility of the branch 2107455-image-field-default to hidden.

kksandr → changed the visibility of the branch 2107455-102 to hidden.

Changes that I made:

1. Auto-wiring of dependencies
2. Dependencies were moved to the properties of the constructor
3. An interface was defined for the new service
4. Access check was moved to the service itself
5. The service uses entity_field.manager to get fields instead of loading only configuration fields
6. The access check now returns an AccessResultInterface, which makes it possible to provide cache metadata if it is to be used somewhere other than a file access check

I also left a few details for consideration tagged @todo.

I moved the new service to the images module because it handles loading default images and checking access to them.

@joseph.olstad thanks for working on this, when I apply the patch in #104 I get this error:
ParseError: syntax error, unexpected identifier "DEFAULT_IMAGE_DIRECTORY", expecting "=" in Composer\Autoload\{closure}() (line 17 of /app/web/core/modules/image/src/ImageFieldManagerInterface.php).

@yospyn this fix targets the 11.x branch, which depends on PHP 8.3, so I used typed constants that are available in PHP 8.3. You can open a separate merge request for 10.3.x and lower the PHP requirements. You can also always update PHP on your server.

@kksandr success! Patch #104 worked with PHP 8.3 on 10.3. No errors in logs. Thank you

I created a merge request for Drupal 10.3.x version and I fixed the PHP8.1 compatibility.

I have some remarks. See the MR

luenemann → changed the visibility of the branch 2107455-image-field-default to active.

luenemann → changed the visibility of the branch 2107455-image-field-default to hidden.

Just rebased onto 55bab340, just before the 📌 [ignore] Convert everything everywhere all at once Active .

I removed some out of scope comment changes to simplify the rebase.

CI pipline is green, Test-only changes fails as expected.

Still "Needs work" for #123 and hook conversion.

oily → changed the visibility of the branch 10.3.x to hidden.

dhaval_panara → changed the visibility of the branch 10.3.x to active.

Adding the patch file instead of using git MR in composer json.

Hooks in test modules should also be converted to OOP.
IMHO that is the only thing that's left.

There is no more subsystem maintainer for image.module since 📌 Remove claudiu.cristea from MAINTAINERS.txt Active . How to proceed?

Thanks for your work. It works for me too for 10.4

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@kksandr, thanks for fixing the merge request!

I created a new merge request because the branch name of the merge request was 11.x which is a confusing sort of branch name as it matches the branch name in origin. Perhaps was an accidental choice. In any case, I suggest moving forward with an appropriately named branch.

kksandr → changed the visibility of the branch 11.x to hidden.

kksandr → changed the visibility of the branch 10.3.x to hidden.

I believe this is ready for a subsystem maintainer review.

A couple comments on the MR. I think there might be missing test coverage for a private default image set on field config instead of field storage config.

Funny enough, the order of operations between && and = was also the problem behind 🐛 Exposed filter values ignored when using batch Needs work .

Funny enough, the order of operations between && and = was also the problem behind #3403999: Exposed filter values ignored when using batch.

@godotislate These are two completely different cases.

LGTM.
Removing Needs Tests and Needs subsystem maintainer review since it looks like both were already done.