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

#2: private-default-image-2107455-1.patch queued for re-testing.

This bug is still there in 8.2.
The only way to use default images with private stream is to set the stream of the image field as public and upload the default image. After that you can set the field stream as private...

This also builds on the tests in #2.

This looks like a nice fix and improvement to the test coverage in these cases. Thanks @ShaunDychko and @claudio.cristea

The solution could be merged as the fix is duplicating a bunch of code for if (strpos($path, 'styles/') === 0) {. Not sure if the refactor is needed but thoughts?

1. +++ b/core/modules/image/image.permissions.yml
   @@ -1,2 +1,5 @@
   +view private default images:
   +  title: 'View private default images.'
   

   Do we really need a new permission for this? If the user has access to the field then they should have access to the default - but how are you going to know what field... ugh.

I think we might have to work out where the default image comes from and check if the user has access to that.

Reroll against 8.5.x. core/modules/image/src/Tests/ImageFieldDisplayTest.php moved to core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php.

Triggered 8.7.x automated tests

The patch in #18 fixes the issue on an image field set to private.

#13/#14 have not really been addressed. Adding a new permission for this feels a bit off when we already have the field access system.

Here's an attempt to check the default image's field access (addressing #13/#14). Right now I'm only checking the field access with Drupal\Core\Entity\EntityAccessControlHandler::fieldAccess(). Not sure if we should also be creating a stub entity and checking access against that. For example, right now, if you don't have the "access content" permission, you'll still be able to see default images.... seems wrong?

Also the tests need work, but I wanted to see if this is the right direction before heading down that road.

#18 worked for me on V8.8.5

Re-rolling #18 patch against 9.1.x-dev version.

@pradeep rest looks good but the minor change required.

+++ b/core/modules/image/image.module
@@ -212,6 +212,24 @@ function image_file_download($uri) {
+      return array(
+        // Send headers describing the image's size, and MIME-type.
+        'Content-Type' => $image->getMimeType(),
+        'Content-Length' => $image->getFileSize(),
+        // By not explicitly setting them here, this uses normal Drupal
+        // Expires, Cache-Control and ETag headers to prevent proxy or
+        // browser caching of private images.
+      );

Please try to use a short array instead.

forex.

return [
  // Send headers describing the image's size, and MIME-type.
  'Content-Type' => $image->getMimeType(),
  'Content-Length' => $image->getFileSize(),
  // By not explicitly setting them here, this uses normal Drupal
  // Expires, Cache-Control and ETag headers to prevent proxy or
  // browser caching of private images.
];

Assigning to myself, as I am working on this.

@jyotimishra123 Can you Please change some code.
#1
1. if (strpos($path, 'default_images/') === 0) to if (strpos($path, 'default_images/') === FALSE)
2. $access_private_default_images_user = $this->drupalCreateUser(array('view private default images')); To $access_private_default_images_user = $this->drupalCreateUser(['view private default images']);

Hi @pavnish -> if (strpos($path, 'default_images/') === 0) to if (strpos($path, 'default_images/') === FALSE) is throwing error. So I've left this change.

Hi, the patch in #41 applied cleanly on Drupal instance 9.1.x and worked as expected!!

Patch #41 working as expected.

Everyone, please read the issue before rerolling patches. #13 and #14 have still not been addressed.

I am removing credit for all the rerolls after #24 because they are not working toward solving the issue. Please read #13 and #14, and try to build on #24. Thank you!

Update the title and issue summary in preparation for a patch.

(hid some irrelevant file uploads)

@pavnish, I'm working on the issue right now; I'm just working on the tests. Sorry for the delay.

I apologize for the delay - I don't get a lot of practice writing functional tests in PHPUnit.

Here is an attempt to update @drclaw's patch from #24 (which attempts to address @alexpott's concerns in #13 and #14).

Reviewers; please verify the completeness and soundness of the access-checking logic, since that has implications for security. In particular, this only checks if the current user has 'view' access to the field; not whether the user has 'view' access to the entity type(s) that the field is embedded in. (Turns out it is difficult to create a user that does NOT have access to 'view content' (i.e.: nodes) in functional tests, so its non-trivial to write a test for that case)

I've also included interdiffs between:

1. this patch and the one in #24, and
2. this patch and the most recent (incomplete) patch in #41

Feedback welcome.

Should set this to "needs review" and unassign myself.

I have applied patch #49 and from what I can tell its fixed my issue. I cant say anything about the code but on the front end it all seems to work.
Thank you to everyone that has been working on this issue.

I messed with this and tried everything I could. Even the Patch failed locally at first. But then I tried a different syntax and it worked out!

So chalk that one up as one more successful field test!

Now my only suggestion is that whomever is in charge of this thread do a simple step by step and clarify and simplify the process!

It really took me a while to figure out which of the patches in #49 I was to apply especially because that seemed to be the most recent and was marked as needing further testing!

And it was unclear which of the patch commands (from https://www.drupal.org/patch/apply) I needed to apply, but git apply -v private-default-image-is-inaccessible-2107455-24.patch run from my root (/html/) folder did the trick after clearing caches!

Note: I used "private-default-image-is-inaccessible-2107455-24.patch" which succeeded. I trust that since it worked that that was the correct one and if not, should I run a different one over ??

I did notice in the production site that one member's profile photo (Open Social Demo User) still showed up on the activity streams, but since she was a demo user, isn't that because their images are derived from a different (Demo) source?

So, I deleted the photo which was correctly replaced with the default, checked the stream as a visitor and indeed the default photo was in place.

Then I uploaded a copy of her photo and tried again and all works fine!!

#49 doesn't apply cleanly on 9.1

patching file core/modules/image/image.module
patching file core/modules/image/tests/modules/image_field_display_test_default_private_storage/image_field_display_test_default_private_storage.info.yml
patching file core/modules/image/tests/modules/image_field_display_test_default_private_storage/image_field_display_test_default_private_storage.module
patching file core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
Hunk #1 succeeded at 443 (offset 1 line).
Hunk #2 FAILED at 478.
1 out of 2 hunks FAILED -- saving rejects to file core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php.rej

Here's the patch in #49, re-rolled. There are no changes, so no interdiff.

I have tested the last patch on 9.2 dev version,the patch applied cleanly but it does not resolve issue

#55 worked on 8.9.12

#55 did not work for me, but it seems to be a step in the right direction.

I am now able to view the default image after uploading it, but it does not display as a default using the layout builder.

Tested on 8.9.13

I ran into this problem again after fixing it several months ago with several Open Social and Drupal updates in between.

Currently running Drupal 8.9.13 and Open Social 8.x-9.9 thru 10.0.3

So I ran the patch again: git apply -v private-default-image-is-inaccessible-2107455-24.patch (Kept in in my /html/ [root] folder) and it resolved the problem returning the default image in every place it is supposed to appear, including Activity Streams and anywhere where the user has not uploaded a replacement!

I am assuming then that since this issue is still in review, that it has not made it's way to the Drupal code/update process and thus may need to be reviewed each time an update is done.

#55 worked for me (manual patch). No errors in watchdog or browser inspector. Path/folders are correct and all styles work. Thank you.

D9.1.5
PHP 7.4.16
Apache - not running Nginx. (May be relevant to this issue?: https://www.drupal.org/forum/support/post-installation/2021-03-29/does-a... )

I would just like to reiterate that until this issue is resolved within Core, the patch does work.

The problem is that every time I do a core update, it is necessary to run: git apply -v private-default-image-is-inaccessible-2107455-24.patch which solves the problem of missing default profile photos.

Hear a reroll patch.

Referencing #62

Drupal 8.9.20 with Open Social (social-10.3.8)

git apply -v private-default-image-is-inaccessible-2107455-24.patch
Renders:

Checking patch core/modules/image/image.module...
error: while searching for:
use Drupal\Core\File\FileSystemInterface;
use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\Core\StreamWrapper\StreamWrapperManager;
use Drupal\Core\Url;
use Drupal\field\FieldConfigInterface;
use Drupal\field\FieldStorageConfigInterface;

error: patch failed: core/modules/image/image.module:9
error: core/modules/image/image.module: patch does not apply
Checking patch core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php...
error: while searching for:
    $cache_tags_header = $this->drupalGetHeader('X-Drupal-Cache-Tags');
    $this->assertTrue(!preg_match('/ image_style\:/', $cache_tags_header), 'No image style cache tag found.');
    $this->assertRaw($default_output, 'Default private image displayed when no user supplied image is present.');
  }

}

error: patch failed: core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php:478
error: core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php: patch does not apply

I am wondering if this has any relationship with: https://www.drupal.org/project/social/issues/3261346

When Logged Out (Stream):

When Logged Out (About Us page):

When Logged in (About Us page:

Neither of the last two patches apply to Drupal core 9.2.12

Added reroll of patch #63 on Drupal 9.2.x.

I apologize if I’m posting in the wrong thread, but on 9.3.9 if I set the file system to private and then upload an image to the core image field (field_image) attached to Article and Basic page it gets saved to the public (sites/default/files/images) system. All other image fields get saved in the private system as desired.

Updated patch will fix test failure. removed decprecated code.

This #72 didn't apply for me on 9.4.1, so rerolled.

Fixed failed commands #74
Attached patch against 9.5.x

Patch #75 tested with success against Drupal 9.4.8
Thanks.

+++ b/core/modules/image/image.module
@@ -169,6 +170,114 @@ function image_file_download($uri) {
+            $file = \Drupal::service('entity.repository')
+              ->loadEntityByUuid('file', $storage_uuid);
+
+            // 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');

$file can be null with imported field_config on a different site where there's no file with that uuid.
In that case you get
Error: Call to a member function getFileUri() on null
for every default image download.

Should wrap it with if (isset($file)) {.

Added reroll of patch #74 on Drupal 10.1.x and made changes as per comment #78.

Successfully applied patch #79 for drupal version 10.1.x. Adding Screenshots for the references

Justification:
Patch has real world success as per #80.
Patch has test coverage.
Patch passes tests.
Fix is needed.

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

Here is a patch for 10.2

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

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.

This patch:

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
• The MR patch also applies (with fuzz) to D10.3.x

@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. ***BEGIN EDIT*** it did light up green after this revert ***END EDIT***

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.
7. Now, instead of caching field IDs, field definitions are cached.
8. A constant has been defined for the default image directory to prevent accidental changes to this directory elsewhere, which could break access checks that rely on the directory name.

I also left one detail tagged @todo for review.

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 cherry-picked the commits and keep the credits and in the end I add a commit to fix PHP compatibility.

Just rebased onto 55bab340, just before the commit of #3483599: Convert all procedural hook implementations to Hook classes.

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.

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 #3501059: Remove claudiu.cristea from MAINTAINERS.txt. 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.

I also triggered a tests only pipeline. Reminder that a fail for a tests only run is success provided that the normal pipeline is green!

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 #3403999: Exposed filter values ignored when using batch.

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.

This could use an issue summary update (well, any issue summary at all), given there are 150 comments and a couple of hundred lines of changes on the MR.

Issue summary was removed in #2107455-131: Image field default value not shown when upload destination set to private file storage. I am going to restore the previous state.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Fix long comment lines

Curious about patch availability for core 11.1.7, tried #131 and the .diff above but no success.

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.

Rebased and resolved merge conflict.

I am going to RTBC, because my changes are only fixing comment coding standard.

There are several comments that are not wrapped correctly. I left suggestions for those in the MR. I didn't apply them because longwave left questions that need to be answered. Therefor, setting to needs work.

Also, I updated credit which is always challenging when there are lots of comments and contributors.

The diff above worked for me when upgrading to core 11.3.2. Have a couple private intranets with user profiles that use the default image, which was previously broken but now appears OK.

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.

Adding the changes as a patch file which is implemented in 11327 merge request the changes are working on Drupal 11.3 correctly.

Test only run shows the coverage https://git.drupalcode.org/issue/drupal-2107455/-/jobs/9055205

Think I'm not 100% sure on is the #Hook() applied to the entire class? Let me ask nicxvan

Think I'm not 100% sure on is the #Hook() applied to the entire class? Let me ask nicxvan

Just to clarify my comment on the MR and the above, there is nothing wrong with Hook attributes on the class, it's fully supported.

I think in core honestly we should prefer consistency and have them on the method so the hook being implemented is immediately apparent when looking at the method.

This one needs a rebase but going to try and make the call can we add the hook to the method to be consistent as mentioned please.

Thanks all!