Color module uses the public file system but doesn't check if it is set

Subscribe.

Is this all that is needed? Using file_default_scheme() instead?

Patch from #3 failed for me in both 8.x and 7.x installations. Steps to reproduce:

1. Install standard installation profile
2. Apply patch - patch -p1 < ~/tmp/drupal-1106160-3.patch
3. In admin menu, choose 'Configuration' > 'Media' > 'File system'
4. Clear value from the Public file system path field
5. Put 'sites/default/files' in the Private file system path field
6. Press Save
7. Choose Private local files served by Drupal as default download method
8. Press Save
9. In admin menu, choose 'Appearance' > 'Bartik' > 'Settings'
10. Choose any color set except default and press save
11. Several warnings appear:
    

    Warning: file_put_contents(public:///.htaccess): failed to open stream: "DrupalPublicStreamWrapper::stream_open" call failed in file_save_htaccess() (line 505 of /home/valeryl/work/d8/includes/file.inc).
        Warning: file_put_contents(public:///.htaccess): failed to open stream: "DrupalPublicStreamWrapper::stream_open" call failed in file_save_htaccess() (line 505 of /home/valeryl/work/d8  /includes/file.inc).

12. Go to the front page - CSS is broken
13. Steps 2-12 repeated for both d8.loc and d7.loc, with the same result

Once a new patch has been re-rolled, we should probably test against simplytest.me (which should help clarify if this is a server/local file permissions issue or an actual bug in the code).

I tested the patch #1106160-6: Color module uses the public file system but doesn't check if it is set successfully as you can see in the following image.

To test this change you must change the settings for any theme like Bartik in URL http://example.com/admin/appearance/settings/bartik and any change create a new folder in public stream folder color.

If you want to review the public stream, you need to go to URL http://example.com/admin/config/media/file-system, as you can read in instructions if you want to change the public stream you must to change in setting.php as you can see in the following code.

/**
 * Public file path:
 *
 * A local file system path where public files will be stored. This directory
 * must exist and be writable by Drupal. This directory must be relative to
 * the Drupal installation directory and be accessible over the web.
 */
# $settings['file_public_path'] = 'sites/default/files';

Looks good to me, RTBC.

Committed cef318b and pushed to 8.x. Thanks!

And now for D7

Backported #6 to D7.

Comparing #6 and #11 side by side the only thing that has changed is the path of the color.module file. Looks good to me.

I tested this and it completely broke the Color module on a site that uses private files as the default download method. The theme doesn't work at all, since the files generated by the Color module are not displayed.

The reason is that Drupal doesn't know about these files so when they are put in the private files directory, it falls back on the default behavior of file_download() which is not to serve them to the browser.

I do not have a working Drupal 8 setup right now, but given @valthebald's test results in #4 (which report basically the exact same thing) as well as reading the code in FileDownloadController::download() I'm pretty convinced the same thing happens there (or if not, then there's likely a critical security issue in Drupal 8 regarding this, because it's what should happen for a random file in the private files directory that Drupal doesn't recognize).

I think this should be rolled back... and perhaps the correct fix is to stop Drupal from allowing you to try running a site without a public files directory in the first place? (Then you wouldn't ever be able to get in this situation.)

I think this should be rolled back... and perhaps the correct fix is to stop Drupal from allowing you to try running a site without a public files directory in the first place? (Then you wouldn't ever be able to get in this situation.)

If this is considered viable I would be in favour of this approach. Right now I have to check if the public files directory exists and is configured etc because my themes and module frequently make use of it, because they're serving those files to the browser, so in general I personally would be in favour of requiring a public files directory for these unmanaged files that are served to the browser.

Reverted for now.

It is no longer possible to follow the steps to reproduce that are in #4. I did play around though on drupal 9.3.x, standard install. I set the download method to private and the set public directory to '' and the private one to sites/default/files and then got "The website encountered an unexpected error. Please try again later." when going to admin/appearance/settings/bartik.

I wonder if this should be a child of #2724963: Make the public file system an optional configuration?

Color has been removed from core, #3270899: Remove Color module from core.