Custom logo and favicon stored in private filesystem if it is the default

I am having the same issue
I am running Ubuntu, LAMP, Drupal 7, Clean install and No Extra Module,

Hi

I've been trying to find the other issue related to Drupal 7, custom logos and private upload (as this issue was closed as duplicated). Seph, could you tell me where is it?

I have the same problem in a fresh install of Drupal 7, with only the MAYO theme installed.

Cheers

Tracking this have same issue with image attachments when using a private folder for images as field attachments.

problems while using pvt files: default logo & favicon doesn't work
Here i have attached a picture of what happens in place of logo

tough i uploaded them in public directory : sites/default/files/
sites/default/files/logo.jpg
sites/default/files/favicon.ico
it worked...

updated pic

Sub

I had this problem too. The issue was that the submit handler for the theme settings form was copying the file to the correct location out of the temp directory, but it was not updating the record in the db. I wrote a patch to fix this. I believe this was a core bug in the system module: it simply didn't manifest itself if you weren't using the private filesystem.

Obviously use this patch in a controlled environment. It hasn't been fully tested with non private filesystems.

#16: it worked when i uploaded custom shortcut icon but only if there is no custom logo...
Custom logo works fine...

babbar, are you using private file system or public?

Uploading a shortcut icon with a logo image already present works fine on my private file system site.

I fixed this patch so it correctly sets the status of the copied logo/shortcut file. When I tested it, it worked when both of these were being uploaded at the same time, or individually.

If we can determine that the patch works, I'll refactor it a bit (right now it's too repetitive).

Having this issue too with Drupal 7.2 (updated recently from 7.0, but still no luck).

Is this the same as #1028092: Default image is not set to permanent and saved to the wrong schema ?

This patch conflicts with #901724: After uploading custom logo for theme the path points to root

And this is a feature request, which I'm actually tempted to mark as won't fix.

I have to agree with Seph. Did you even read post 16 and 19, Sun? It's a bug in core functionality that doesn't do what it's intended to. There is even a patch to work from already here in the thread. Hopefully it just needs to be tweaked to fit with 7.2.

I have just installed D7.2 on MAMP local testing environment. The very first thing I did was go in and set a path for private files, then change the default method to be private files. Then I went into the default theme bartik and tried to upload a custom logo.

After browsing to my logo and uploading, this is the path set to my new custom logo:

private://mylogo.jpg

The system is not reading this file path - the logo does not display.

Just posting this in the hope this issue will remain open and move towards solution.

Can't believe this hasn't been fixed in 7.2 ...

Bugs are fixed in head first and I agree with Sun this is a feature request

This needs to be fix in d8 first if indeed it is a bug.

Thanks to #33 I finally understand the problem.

The previous issue title as well as earlier patches in the issue suggested a bogus problem and a bogus solution. Namely, allowing to deliver favicon, logo, etc from the private filesystem. Since files from the private filesystem are delivered via Drupal (and not the web server), that would imply a huge performance decrease. That's why I marked it as a feature request.

However, the actual problem is that System module does not enforce the public filesystem, so files are stored in the private filesystem if it is the default.

To move forward here, we need the simple steps to reproduce from #33 in a test.

To move forward here, we need the simple steps to reproduce from #33 in a test.

why do you need to reproduce this?

I would have thought that the example of a clean install of 7.2 that I provided (#27) would suffice? Who needs to reproduce it to move forward - is there anything else that can be done to help?

"To move forward here, we need the simple steps to reproduce from #33 in a test."
Read this again please....

We need the actual steps (the clicks on specific pages) to create this bug.

WHY?
Because we need to write a test for this.

WHY?
Because after implementing the test, the testbot should say that drupal core is broken.
When we have a failing test we can try to fix drupal core so that it won't fail anymore.
With the test we can verify it actually works.

The actual steps to reproduce are clear and known already. See #33.

An automated test is required to confirm that the test is currently failing, to confirm that a proposed resolution fixes the bug, and lastly, to prevent us from re-introducing this bug in the future.

It's pointless to argue for or against this. Write the test and submit it as a patch instead, please. Don't have time to work on this myself currently, otherwise I would.

Yeah, this is still an issue in 7.x. Right after a fresh install, if you (a) go direct to the file system and enable the private file system and then (b) go to the theme or global theme settings and upload a custom logo and icon; then neither will work. Both upload fine as can be seen if you ssh (or ftp) into you private files directory. As a work-around, I just copied them (via ssh) from the private files directory to the public files directory and then updated the path in the theme to reflect the public path. It's a work-around but not everyone will have the access to do this.

My 2c worth is that this should not be regarded as a feature request, as the private file system is potentially very useful for me. I have not yet had a look to see how complex this is to fix, but I would be a very grateful puppy is this was fixed.

Here's an updated patch from #19.

Please ignore #43. Here's the actual patch. Sorry about that!

Tagging.

Looks straightforward enough. We still need a test that fails without the patch and passes with it.

I wrote a patch which seemed to should fail, but it didn't.
Additional it adds tests for favicon.

+++ b/modules/system/system.test
@@ -1603,28 +1603,58 @@ class SystemThemeFunctionalTest extends DrupalWebTestCase {
-    // Upload a file to use for the logo.
...
+    // Upload a file to use for the logo

Please revert. ;)

+++ b/modules/system/system.test
@@ -1603,28 +1603,58 @@ class SystemThemeFunctionalTest extends DrupalWebTestCase {
+    // Setup a private file system, both the logo and the favicon should still be uploaded to public, so people can actually see it.
+    variable_set('file_default_scheme', 'private');
+    $logo_file = current($this->drupalGetTestFiles('image'));
+    $favicon_file = current($this->drupalGetTestFiles('image'));

I wonder whether we're not missing a next() here? Test might be passing because we're using the same files for the public vs. private test?

Minor: Comment should wrap at 80 chars.

6 days to next Drupal core point release.

Thanks for the feedback!

I wonder whether we're not missing a next() here? Test might be passing because we're using the same files for the public vs. private test?

You are right about this, and this could be even a problem of the current way this test works.
If it's upload the same image twice it's just "random" that it's work.

Here is a new patch, but it still doesn't fail, sadly.

The comment part is corrected.

Hm. Might be a testing framework issue? Or, can we actually reproduce this bug manually?

Subscribing. No doubt making ALL file requests private is a performance hit. Is it possible to nominate some areas of the file space as private, leaving others public? E.g. by content type? Or by some user-authorisation-level / ACL field?

My use case is as follows. I am using Drupal to provide learning management system LMS capabilities. Here, two basic requirements are:

1. That students and teachers have access only to relevant materials
2. That work submitted by one student (or team of students - Organic Groups) be visible only to that team (where team includes also the teacher, so that she can assess the student work).

Since these requirements encompass a large majority of the site content, it is reasonable to make private files the system default and pay the performance price.

By contrast, I really don't care whether or not the logo is publicly visible!

And yes, I know that there are perfectly good LMS available elsewhere. I and others just want to do this using Drupal, where we can control what we get! See for example http://groups.drupal.org/lms-learning-management-system

I can and will live without a logo. But is there other similar functionality loss elsewhere in Drupal when private files are enabled?

Many thanks for your work to patch this issue.

This is NW if the test doesn't fail, right?

Related: #1376166: Custom logo and favicon functionality inanely tries to support absolute local file paths

I just experienced this same issue.

I installed the patch (thank you very much!) and it works great on the homepage.

However, I cannot upload logos onto my affiliate sites made through Domain Access. I get the same private filesystem error. So, the logo appears on the homepage, but I cannot upload custom logos for my affiliates.

Any guidance you can provide on a fix?

1) I don't think this is major. You need to have a private filesystem in the first place. And you can very easily work around the issue by uploading your logo/favicon to your host.

2) We should still start with tests. #1376166: Custom logo and favicon functionality inanely tries to support absolute local file paths landed for D8 in the meantime. Contrary to the patch in #48, I'd prefer to perform this test in a separate test method; i.e., not mixing the code into an existing test method. That is, because this test is very specific and different to the existing theme settings assertions.

As far as I can tell this is/was still a problem with Drupal 7.23 (I know Drupal 7.24 is the latest). I appreciate that this is not a problem if you have access to the server, but I had delegated a site to a Drupal admin (only, no access to server) and it was a problem for him.

A simple workaround (tested) is to simply switch temporarily back to the public filesystem, browse under the theme settings for your logo and upload, then switch back to private filesystem.

Cheers webel - very timely!

I had this problem today and your solution was just what I needed.

We're still using this patch (from #44). It stopped working as of Drupal 7.33 or 7.34. Here it is again for 7.34.

I just encountered the very same issue with DP7.36. Daring as I am I tried the patch (#57) but it was to no avail.

Sorry, I have no idea what the status of this is for D8.

However, thank you for D7 patch in #57. It applies without difficulty to drupal 7.38 and works as expected allowing custom logo and favicon (shortcut) image files to be uploaded as if public even when file system is set to private.

Thank you for D7 patch in #57. It applies without difficulty also to Drupal 7.39

Thanks, I just used patch #57 for openatrium 7.x-2.50. I have a fresh install with a private file system configured. I created a sub-theme from oaradix and made it the default. I disabled the OA branding module, which left me with the Radix logo. I attempted to upload my own logo. The logo was placed in the private directory - reloading the site displayed no logo (but no errors). I applied the patch directly from the file's directory: /var/www/html/modules/system$ patch < 1087250.logo-public-filesystem.057.patch

Re-uploaded the logo and problem solved. Thanks again.

patch in 57 introduces ';;' so it needs to be reworked

i just updated the patch to remove the small error.

I've created a simple patch for the 8.3.x or 8.4.x branches which forces the uploaded logo or favicon file into public storage if by default Drupal is set to use the private storage. I can't believe that it's that simple though.

The solution in #70 is really, really simple and that's a good thing. I don't see why that wouldn't work. If we get more buy-in on that, we should add tests for this, to ensure that this behavior doesn't regress in the future.

Even if it would be possible to find the favicon, it would be a major performance penalty to pay.

Potentially two extra bootstraps are not necessarily cheap.

The patch at #70 no longer works with Drupal 8.6.16 so here is an updated patch.

This won't work for 8.7.x as the processing of the uploaded logo and favicon has changed completely. I've not checked if the underlying issue is still there with 8.7. This issue is still present in 8.7. The logo is uploaded to the private file system and is then just returns a forbidden error. Hopefully I will get around to producing a new patch soon when I have to update this particular project to use 8.7.

Updated patch for Drupal 8.7.1. This will change the file schema to 'public' if the default is 'private'.

Accidentally uploaded twice, the first time with a test against the wrong version.

I'll get this right at some point. This should be the patch tested against the correct version.

Setting back to Needs Review!
This is not happening anymore.

An updated patch for 8.8.x as file_default_scheme() is deprecated.

Closed #47709: private://logo.png gets a 403 for custom logos as a duplicate of this one. Although that was earlier than this issue, this issue has more discussion. Moving credit here.

Adding a test. There is no interdiff with the previous test because the change is so small and the interdiff here between the fail patch and the success patch is sufficient.

Still to do:
This needs before and after screenshots added to the Issue summary.

Removing a couple of unused use statements

Applied patch #92 and it's working correctly as proposed resolution which is, always having public scheme for the logo.
@here adding the before and after ss of the patch.

@NitinLama, thanks for the screenshots! When completing screenshots for an issue, remove the 'needs screenshots tag' to keep the issue up to date. It will also prevent others who are looking to make screenshots spending time reading this issue when the work is already done.

I think the screeshots should be added to the Issue Summary and replace the 'TBA' for the before and after screenshots. Add any comments you think will help reviewers/committers. When that is done the tag for 'Needs issue summary update' can be removed.

Setting to NW for the Issue summary update.

Here Updated the issue summary.

@NitinLama, thanks.

This now has an up to date Issue summary, screenshots, a fail patch #89, and a cleaned up patch #92.
This is ready for review.

Tested patch #92 on 9.5.x

The patch applies cleanly and allows uploaded files destination to be fixed to public scheme even when the default file scheme is set to private. Confirm it fixes the issue following the steps to reproduce.

Updated the test from #92 for deprecated functions and theme bartik to olivero on 10,
file_create_url()
file_url_transform_relative()

Deprecation notice for function: https://www.drupal.org/node/2940031
Deprecation notice for bartik: https://www.drupal.org/node/3223395#s-bartik

Staying put in NR for a review on new patch.

I'm not sure this is the right fix. If the default file system is private why is the logo and favicon any different? Is there a legitimate use case for the logo and favicon to be private? Perhaps to show a different image to different users, although in that case there would need to be other customisations as well.

Ignoring that, a couple minor points below.

1. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -460,6 +460,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // Can't use private for these files as they will not be accessible.
   +    $default_scheme = ($default_scheme === 'private') ? 'public' : $default_scheme;
   

   This ternary is a bit awkward. How about this:

   if ($default_scheme === 'private') {
     $default_scheme = 'public';
   }
   

Do we need test coverage for the favicon too or just the logo?

Providing new failtest for updated patch.

Forgot to mention the test on #92 fails for array structure assertion as well before any deprecation fix, this failure was fixed in #104.

Tried to fix the CCF in #106.
Please review.

Hi @_utsavsharma,

#106 is a purposely test that fails to prove the test asserts the issue properly when the actual fix applies, so the patch content have its fix removed, the patch on #104 contains the actual fix.

It will be helpful if you're willing to adjust the patch on #104 too, thanks.

Hi @mstrelan,

In Drupal 8, private file permissions by default are given according to the entity to which they are attached. If the user has permission to view the entity, they will have permission to view the file.
- Jaypan on Drupal-stackexchange

Past discussion on #50 described their edge use case on why they needed restriction on private scheme (but not particularly for logo and favicon). But core doesn't provide a proper permission control for private files to be viewed without further alteration by contrib/custom module, which is rather confusing for theme uploading files through settings page. (as mentioned in this discussion Drupal Forum)

If what Jaypan stated on stackexchange is still true at D10, to fix it following the logic of Drupal, we need to allow both file field (logo and favicon) to respect theme's viewing permissions. Is this applicable?

Do we really need logo and favicon files to be set in private folder? Or choosable for users? I'd side with putting it under public scheme with the current approach.

Yes I agree using the private file system for this is setting you up for failure, just trying to see if there is a valid reason to do it.

Update fix by #105, add test for favicons file field.
Fixed CCF error.

Cool, then we shall keep this approach for now till a second opinion gives otherwise.

Set to NR.

Fixed CCF error for #111.
Please review.

Thanks for the help @_utsavsharma!

I did review the patch in #111 and #112 and I have a question here

Why don't we specify the $scheme = 'public'; instead of all the checks below?

$default_scheme = $this->config('system.file')->get('default_scheme');
+    // Can't use private for these files as they will not be accessible.
+    if ($default_scheme === 'private') {
+      $default_scheme = 'public';
+    }
+