Problem/Motivation
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?
This issue was committed to 8.x and reverted.
Steps to reproduce
TBA
Proposed resolution
TBD
Remaining tasks
TBS
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#11 | color-1106160-11.patch | 622 bytes | dcam |
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedSubscribe.
Comment #2
BarisW CreditAttribution: BarisW commentedOn 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?Comment #3
tim.plunkettIs this all that is needed? Using
file_default_scheme()
instead?Comment #4
valthebaldPatch from #3 failed for me in both 8.x and 7.x installations. Steps to reproduce:
patch -p1 < ~/tmp/drupal-1106160-3.patch
Comment #5
markhalliwellOnce 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).
Comment #6
BarisW CreditAttribution: BarisW commentedHere's a re-roll.
Comment #7
-enzo- CreditAttribution: -enzo- commentedI 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.
Comment #8
markhalliwellLooks good to me, RTBC.
Comment #9
alexpottCommitted cef318b and pushed to 8.x. Thanks!
Comment #10
alexpottAnd now for D7
Comment #11
dcam CreditAttribution: dcam commentedBackported #6 to D7.
Comment #12
markhalliwellComparing #6 and #11 side by side the only thing that has changed is the path of the color.module file. Looks good to me.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.)
Comment #14
Jeff Burnz CreditAttribution: Jeff Burnz commentedIf 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.
Comment #15
catchReverted for now.
Comment #17
catchComment #30
bradjones1Comment #31
quietone CreditAttribution: quietone as a volunteer commentedIt 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?
Comment #32
bradjones1Comment #35
quietone CreditAttribution: quietone as a volunteer commentedColor has been removed from core, #3270899: Remove Color module from core.