Line 332 in color.module does
$paths['color'] = 'public://color';

However, Drupal allows me to only enter a private file path in my file system settings, and leaving the public files directory empty. Drupal works fine, until I try to change the colors of my theme. Then Drupal generates some bugs as

The file permissions could not be set on public://color.

and

File temporary://fileCUgZxH could not be copied, because the destination directory is not configured correctly.

Is it possible to use the private file system, if the user prefers that? Of if not, just check if the public file directory is set?

Files: 
CommentFileSizeAuthor
#11 color-1106160-11.patch622 bytesdcam
PASSED: [[SimpleTest]]: [MySQL] 40,864 pass(es).
[ View ]

Comments

Jeff Burnz’s picture

Subscribe.

BarisW’s picture

On a side note, I discovered this while fixing a bug in my Google Fonts module (#1103884: system/files/google_fonts/google_fonts.css page not found). It seems that /system/files/name_of_my_css_file.css resolves in a 404, so maybe that's why the color module hardcoded public:// in it?

tim.plunkett’s picture

Version:7.x-dev» 8.x-dev
Status:Active» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new622 bytes
PASSED: [[SimpleTest]]: [MySQL] 32,931 pass(es).
[ View ]

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

valthebald’s picture

Status:Needs review» Needs work

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
markcarver’s picture

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

BarisW’s picture

Status:Needs work» Needs review
StatusFileSize
new642 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,603 pass(es).
[ View ]

Here's a re-roll.

-enzo-’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new138.22 KB

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.

color-with-default-public-stream.png

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';
markcarver’s picture

Looks good to me, RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed cef318b and pushed to 8.x. Thanks!

alexpott’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

And now for D7

dcam’s picture

Issue summary:View changes
Status:Patch (to be ported)» Needs review
StatusFileSize
new622 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,864 pass(es).
[ View ]

Backported #6 to D7.

markcarver’s picture

Status:Needs review» Reviewed & tested by the community

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.

David_Rothstein’s picture

Title:Color module uses the public file system but doesn't check if it is set» [Regression: Color module broken on sites that use private files by default] Color module uses the public file system but doesn't check if it is set
Version:7.x-dev» 8.x-dev
Priority:Normal» Critical
Status:Reviewed & tested by the community» Needs work

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

Jeff Burnz’s picture

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.

catch’s picture

Priority:Critical» Normal

Reverted for now.

  • Commit 46e2561 on 8.x by catch:
    Revert "Issue #1106160 by tim.plunkett, BarisW: Fixed Color module uses...
catch’s picture

Title:[Regression: Color module broken on sites that use private files by default] Color module uses the public file system but doesn't check if it is set» Color module uses the public file system but doesn't check if it is set