Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The solution to enabling the color picker to work when the file download method is private is to put the colors in the configuration directory instead of the files directory. Having all of a site's settings files in one directory makes more sense, too.
Comment | File | Size | Author |
---|---|---|---|
#73 | z7tools.zip | 1.23 KB | z7 |
#71 | css+js-aggregation-D6.17.patch | 12.13 KB | guidot |
#39 | 2009.09.07-public_misc03.patch | 12.49 KB | arhak |
#38 | 2009.09.07-public_misc03.patch | 12.49 KB | arhak |
#32 | 2009.08.25-public_misc02.patch | 12.49 KB | arhak |
Comments
Comment #1
Darren OhPatch attached.
Comment #2
Darren OhComment #3
cog.rusty CreditAttribution: cog.rusty commentedJust for not repeating things which have already been said, a link to the old discussion:
http://drupal.org/node/92059
+1 for the idea of an always public place for some Drupal generated files, and the conf directory seems the right place for this.
It is a limited implementation of Steven's case (1) in http://drupal.org/node/92059#comment-151727 Not yet a "hybrid download method" but it would serve well.
Perhaps it can take some generalization. Giving to an optional module like 'color' a whole subdirectory of its own directly under 'sites' seems... asymmetric. What about 'drupal_files/color'?
Comment #4
Darren OhIt doesn't get its own directory under sites. It gets a directory under the configuration directory for a particular site. For example:
I would avoid putting configuration files in a files directory from now on. The case for having both public and private file downloads doesn't need to include configuration files.
Comment #5
cog.rusty CreditAttribution: cog.rusty commentedI agree that this kind of files can be completely separated from the user files.
That was a typo. I meant the same thing. You have:
sites/example.com/modules
sites/example.com/themes
sites/example.com/color
Not to disparage the color module, but it doesn't deserve that place. It also sets a bad example for other modules' developers, who may feel like adding a few directories there. On the other hand,
sites/example.com/drupal_files
can see eye to eye with 'modules' and 'themes' and can contain 'color'.
Comment #6
Darren Ohdrupal_files is an awkward name to me. If we decide that to always use a files directory in the site's configuration directory, instead of the directory returned by file_directory_path(), I think the problem would be solved. Even if it is used as the user files directory and downloads are set to private, files in that directory are actually public.
Comment #7
drummI do not think this is a good change for the stable 5.x branch. There is quite a bit of infrastructure around helping users get the permissions right on the usual file upload directory, such as the line in admin/logs/status. None of this, except for a bit around the installer, exists for the sites/... directory.
Additionally, we really do not want to have files being written to the sites/... directories outside of the installer and should keep the filesystem permissions read-only. Any potential writing to settings.php or encouraging surrounding files to have less-secure filesystem permissions would be a security problem.
Comment #8
Darren OhThe color module requiring people to make their files directory public is also a security problem.
Comment #9
hass CreditAttribution: hass commentedIt looks like we get private and public files handling in D7... so maybe won't fix!?
Comment #10
Gábor HojtsyOnly won't fix once we really have public/private files at the same time committed to core :)
Comment #11
PanchoThis is still active and it is a security problem. Even that late in the release cycle, we should try to get this fixed.
Drumm's argument in #7 doesn't seem to apply anymore, as we already moved the files folder to sites/default/files.
Adding another folder sites/default/system-files for system generated files (at the moment: color.module) is no additional security risk. We should do that.
Comment #12
PanchoThis is not only about color.module, but also CSS and JS aggregation. We should put these files as well into sites/default/system-files.
Comment #13
Gábor HojtsyIntroducing a new folder for system generated files is unfortunately out of question in Drupal 6, since it is quite late.
Comment #14
PanchoOkay. I also found in a related issue a striking argument against private download method for all these files (color.module, aggregated css and js):
Steven pointed out that
Too bad, otherwise it could be solved by a temporary switch to public download method.
Still, this is a major security, functionality and usability bug, that I was tempted to mark critical (Only left it at normal because of my record of false critical issues lately... :). This is no new functionality and we don't necessarily need to introduce new strings, so I don't really see the point in postponing this to D7.
Comment #15
hass CreditAttribution: hass commentedWe hopefully get private and public download in D7... in 2007 we have had a Google Summer of Code student working on it...
Comment #16
gpk CreditAttribution: gpk commented@Pancho: Don't forget there is a "fudged" work-around for simultaneous private and public downloads that works in D5 and should in D6 - namely, with private downloads enabled, keep the files folder within the web root and use URLs such as /files/... (or /sites/example.com/files/...) for public download and /system/files for private. Obviously this has severe limitations (not least that system generated URLs are nearly always private and also that if you know how then you can get any private file using "public" http access) but for some sites it is adequte.
Then to use color.module you can temporarily enable public downloads, do your color.module stuff, switch back to private, and the updated color scheme still works :-) since it is one of the rare instances when the generated URLs are always "public"; of course you can't change the color scheme unless you go back to private. See also http://drupal.org/handbook/modules/color#comment-657434.
BTW I think drumm's comments at #7 still largely apply since it is only sites/example.com/files that is left writable by the server. There was enough hassle making just this one folder work nicely/be writable etc. during the installation process. See http://drupal.org/node/194369.
Comment #17
zeno of elea CreditAttribution: zeno of elea commentedI was redirected here by the bottom of "lets not reopen this old issue"
http://drupal.org/node/92059
It's not an old issue. I got drupal 6, unzipped and uploaded to my webserver. Immediately I have a problem with the files directory as it's owner is listed as apache, I change the owner to my web user and it works. Then if I try and change the color of garland my theme disappears.
?!?!?
Comment #18
cog.rusty CreditAttribution: cog.rusty commentedThe expected behavior of the color module (or any other php script which writes files) is to try to create its subdirectories and files as the web server user, usually 'apache' or 'nobody'.
To succeed, your files directory must be:
- either owned by user apache and having the normal 755 or 775 permissions
- or owned by your own user account and having 777 permissions (Watch out for this!)
Some, but not most, server configurations are more permissive.
The same must be true about your temp directory.
Additionally, php must not be configured in "safe mode", or else the operation may fail anyway (depending on other details of the configuration).
Is this still an issue?
Comment #19
birdmanx35 CreditAttribution: birdmanx35 commentedThis is still an issue- it's still referenced in core :P
Comment #20
birdmanx35 CreditAttribution: birdmanx35 commentedComment #21
catchNo longer applies.
Comment #22
catchComment #23
arhak CreditAttribution: arhak commentedsubscribing
Comment #24
arhak CreditAttribution: arhak commentedWhat about JavaScript translation? They seems to break when download method is set to private. The content is actually translated and the .js file is saved, BUT in a private folder.
I think is crucial to have a public directory under installation to handle several cases generally related to css/js and nothing to do with the concept of a private download method.
I wasn't aware of this discussion until today (also it's titled color.module which is the less important issue because it's a matter of fanciness), so I did a proof of concept for js aggregation and js translation.
I apologizes for the decisions taken weren't the best thought, but it was just a matter of testing. So, I provided a working patch for Drupal 6.3 at http://drupal.org/node/296831#comment-969381 which fixes the following three issues:
1- css/js aggregation
2- color.module
3- js translations
Note: it's not posted as an issue because it's not intended to be adopted neither proposed as a patch, instead it's for testing it in a D6.3 installation and measure how huge would be introducing similar considerations into core.
BTW: the three solved issues can be tested separately or together, to get the whole picture start reading from http://drupal.org/node/296831#comment-969355
Comment #25
bdragon CreditAttribution: bdragon commentedSubscribing, this affects gmap as well.
Comment #26
arhak CreditAttribution: arhak commentedseems that this issue is going nowhere
patching color.module won't solve many other related issues
is just one big issue: private download doesn't have nothing to do with some public css/js/images from other modules like core's js/css aggregation, js translation, color.module, and now gmap comes up too.
So, thinking about solving the problem from it's root?
or will the issue queues will full of similar "private download method" issues?
PS: Although think this should be ported to D6, currently I'm working with my own patch to core (which at the time was for D6.3)
Comment #27
captaingeek CreditAttribution: captaingeek commentedissue expirenced here clean install of drupal 6.1 on iis7. attempting to change the garland theme color breaks theme until reverting back to the default color brings back the default theme but still can't change colors.
I get the following when attempting to change colors.
The configuration options have been saved.
The directory sites/default/files/color/garland-0d57831b has been created.
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
status page
Drupal 6.10
Access to update.php Protected
Configuration file Protected
Cron maintenance tasks Last run 23 min 42 sec ago
You can run cron manually.
Database updates Up to date
Drupal core update status No update data available
No information is available about potential new releases for currently installed modules and themes. To check for updates, you may need to run cron or you can check manually. Please note that checking for available updates can take a long time, so please be patient.
File system Writable (public download method)
GD library bundled (2.0.34 compatible)
MySQL database 5.1.31
PHP 5.2.8
PHP memory limit 256M
PHP register globals Disabled
Unicode library PHP Mbstring Extension
Update notifications Enabled
Web server Microsoft-IIS/7.0
Comment #28
zeropaper CreditAttribution: zeropaper commentedI completely agree with #24 and deadly would like to see such feature being backported (if implemented in 7.x) within 6.x.
By the way, I guess having a public (for files who does not need authentication) and a private folder could spare some server resources.
Comment #29
arhak CreditAttribution: arhak commentedanother issue raised due to private download method #423652: Private file system can't see crop_display.jpg image.
Comment #30
arhak CreditAttribution: arhak commentedcontributed alternatives:
private_upload
private_download
Comment #31
arhak CreditAttribution: arhak commentedpath commented at drupal.org/node/296831
this version was against Drupal 6.12, but I think it's the same for 6.13
read comment #24 of this thread for description
Comment #32
arhak CreditAttribution: arhak commented@#31 missing files in the patch, now fixed
remember that this patch requires the creation of
misc/dynamic
directoryif the site was already running with other languages set,
javascript translations might be pointing the wrong location
therefore, a cleanup might be required:
- SQL
UPDATE languages SET `javascript` = '';
- PHP
locale_inc_callback('_locale_rebuild_js');
Comment #33
talino CreditAttribution: talino commentedPardon the newbiness: I need to apply this patch because my JS translation file's link is revealing my host's full path to the private files folder in the HTML source. However, I've never applied a patch before. I tried this in my (local) root install dir:
But I got this in return:
Do I need to modify the 'a' and 'b' to point to something? And if so, what?
Thanks a lot.
Comment #34
arhak CreditAttribution: arhak commented@#33 sorry, I made the patch with GIT (actually with Mercurial), I'll be back to you as soon as I can
Comment #35
talino CreditAttribution: talino commentedThanks for replying quickly. I think I managed. After going through the .patch file and looking for stuff to be changed, I copied all the needed files into a folder called 'a' in which I've created the Drupal structure, like this:
/includes/common.inc
/includes/file.inc
/includes/locale.inc
/modules/system/system.admin.inc
/modules/locale/locale.module
/modules/color/color.module
Then I copied the modified and .orig files created by the patch back into the Drupal folders. Created the /misc/dynamic folder and enabled write permissions.
Not the easiest way to do it, I suppose, but it worked.
Semi-worked, really, because I got rid of the JS translation problem which was my major concern (as in "the full unix path to my private folder displayed in the HTML source"). CSS & JS aggregation doesn't work for some reason (radios are enabled, though, in the admin interface) but I can live without it.
All the issues I've read so far concerning Drupal core pale in comparison to this one. A core module that doesn't handle Private Downloads and shows a full unix path in source? Hmm...
Thanks for your help.
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedDoes that still apply to D7 now that the stream wrappers are in? If not, please requalify as D6 only.
Comment #37
arhak CreditAttribution: arhak commented@Damien Tournoud: getting confused, this issue has been changes fron D6 to D7 forward and back, already don't know whether would be right to change it again neither opening another issue, since they got marked as duplicated
according to #13 (which I agree) this patch won't be committed to D6 HEAD, since it introduces new features
nevertheless, I think there is another issue for public/private download support on D7 branch
Comment #38
arhak CreditAttribution: arhak commented@talino#35 I'm sorry to hear that you got so much trouble
here are the facts:
- I don't have CVS, I use Mercurial
- right now I'm working on Windows (my Linux PC will be unavailable for a while)
- the patch I provided was original made for Drupal 6.3, I recently got back to it on demand and it needs to be reviewed again, I'm attempted a quick remake for Drupal 6.13, but obviously it will need some work
Now I will attach the corrected patch
nevertheless it was made with diff (on Windows) which means:
1- the patch will get done without headaches
2- some warnings will show up but won't affect the patch process (e.g:
(Stripping trailing CRs from patch.)
,succeeded at 1312 with fuzz 1.
and(offset -1 lines).
), which actually means it is not a 100% correct patch, but patch command is wise enough to handle itLooking forward:
If you give me feedback I'll fix every feature the original patch was addressing
are you interested on it?
Comment #39
arhak CreditAttribution: arhak commented@talino#35 I'm sorry to hear that you got so much trouble
here are the facts:
- I don't have CVS, I use Mercurial
- right now I'm working on Windows (my Linux PC will be unavailable for a while)
- the patch I provided was original made for Drupal 6.3, I recently got back to it on demand and it needs to be reviewed again, I'm attempted a quick remake for Drupal 6.13, but obviously it will need some work
Now I will attach the corrected patch
nevertheless it was made with diff (on Windows) which means:
1- the patch will get done without headaches
2- some warnings will show up but won't affect the patch process (e.g:
(Stripping trailing CRs from patch.)
,succeeded at 1312 with fuzz 1.
and(offset -1 lines).
), which actually means it is not a 100% correct patch, but patch command is wise enough to handle itLooking forward:
If you give me feedback I'll fix every feature the original patch was addressing
are you interested on it?
Comment #40
talino CreditAttribution: talino commentedThanks for helping out, arhak. I will be hopefully testing this tomorrow morning, Paris (France) time, and will add a comment as soon as I have anything useful to say.
Comment #41
arhak CreditAttribution: arhak commentedI just test it and also reviewed the code
everything looks fine
it works for me
we should troubleshoot your case
are the folders misc/dynamic/css and misc/dynamic/js created once you enable css/js aggregation, maybe it wasn't able to write into misc/dynamic to create those folders
Comment #42
arhak CreditAttribution: arhak commentedBTW patch #32 works fine using
patch -p1 < 2009.08.25-public_misc02_0.patch
note that the
-p1
discards the first element in paths, so you don't have to trick anythingPS: I recommend using patch #32
there is nothing wrong with patch #38 (it is just the same) but was intended to fix Windows encoding and use UTF-8 instead, which leads to wrong source tracking on Windows making the whole files appear as changed (#38 was also fixed to work with
-p0
option instead of required-p1
for #32)always remember to manually create
misc/dynamic
folder AND ensure that directory is writable, since it will need to create subdirectoriescss
andjs
for css/js aggregationComment #43
arhak CreditAttribution: arhak commentedfor those looking for workarounds for private download support:
according to issue 146611#comment-1182361 at #146611: Allow JS/CSS aggregation in private mode D7 is expected to ship with public/private by file functionality, so creating a new folder for D6 was denied
nevertheless, making a public folder available to support features unavailable for private download method is a very affordable cost
so I'll be maintaining this as a path for D6 over this thread
Comment #44
arhak CreditAttribution: arhak commentedother related issues (for those doing some research):
#146611: Allow JS/CSS aggregation in private mode
#166759: Public/Private File Handling
#296831: Enabling css/js aggregation, color module and/or JavaScript translations for the private download method
#457186: Private download method and javascript aggregator
#547400: Compatibility with private download method
Comment #45
arhak CreditAttribution: arhak commentedjust updating the issue state
Comment #46
talino CreditAttribution: talino commentedI'm happy to say that patch #38 worked fine when run from the Drupal root using:
JS Translation link is gone, CSS & JS aggregation are enabled *and* functional. CSS & JS folders created in misc/dynamic.
Well done arhak! And thanks a lot for your work.
P.S. Will comment in this thread if there are any problems but everything seems OK on my end.
Comment #47
arhak CreditAttribution: arhak commentedwhat did you mean with "JS Translation link is gone"?
Comment #48
arhak CreditAttribution: arhak commented@#47 now I recalled and read above, it was the reason for #33 ..because my JS translation file's link is revealing my host's full path to the private files folder in the HTML source.
so I already test it too
Comment #49
hass CreditAttribution: hass commentedNever make your own patches RTBC, please.
Comment #50
arhak CreditAttribution: arhak commented@#49 sorry about that, but I have seen the issues marked RTBC by authors once it is confirmed by them self and another user, and talino confirmed it at #46
also I have this patch running since Drupal 6.3 without any problem
it seems to me pretty much tested
also:
- last contribution to this thread (previous mine) was in October 7, 2007
- last interest before talino at #33 was in May 4, 2009 - 05:23
- I don't thing any body will come up with testing this issue besides the ones sticking with
Comment #51
talino CreditAttribution: talino commentedSorry about #46 being unclear. "JS Translation link is gone" should've been "Wrong JS Translation link is gone", of course.
Comment #52
hass CreditAttribution: hass commentedA "Feature" cannot get into D6... so this may never go in - until it's a bug. New features only go into head and are backported afterwards. In past it was a bug, I don't know why you have changed it. I think you need to find a trustworthy core reviewer. :-)
Comment #53
arhak CreditAttribution: arhak commented@#52 please, it is a long story, read the whole thread
it is a bug which won't be fixed on D6, it is a new feature on D7 and that's why it won't be backported
#11 January 15, 2008 the issue targets D6 branch
#13, #14, #15 switch between D6<->D7 back and forward, but ends in D7
#26 August 18, 2008 I got involved, generalized the issue and pointed it as a "going nowhere" issue and left a link to the tweaking patch for D 6.3
#28 May 4, 2009 last activity before I started providing the path on this thread
#31 August 26, 2009 I attached the (previously referenced) alternative patch
#36 asked to qualify the issue as D6 only, due to existing D7 issues
#37 I start getting lost with what would be the right thing to do, and from this point I decided to take care of this patch providing proper update to it, it won't be ever committed to D6 head, which is not a reason for stopping providing it
please, let this issue be, the same as #7881 has lived for a long while being a new feature hacked into core
for actual D7 issues and other duplicates you may follow link at
#43
,
Comment #54
hass CreditAttribution: hass commentedSounds like we need to give this case a WONT FIX status. Feal free to change it yourself. We do not need stale cases in the queue that never get fixed.
Comment #55
Damien Tournoud CreditAttribution: Damien Tournoud commentedI asked in #36 to clarify if this issue still applies to D7, given the recent changes to the file handling. Bumping to D7 until we have a confirmation that this is not an issue anymore.
Comment #56
Damien Tournoud CreditAttribution: Damien Tournoud commentedClarifying the title.
Comment #57
arhak CreditAttribution: arhak commented#55 no, please read
#43
,
if you mark this as D7 it would become a duplicate
for those interested on a D 6.13 patch go to
#39
Comment #58
arhak CreditAttribution: arhak commentedjust making the title contains "private download method" string
Comment #59
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease answer my question: given that we have now a stream-wrapper based file handling in D7, are the color module, JS translations, etc. all work flawlessly regardless the "Default download method"?
Comment #60
arhak CreditAttribution: arhak commented@#59 I really don't know, I haven't test it, I'm exporadically testing D7 tarball, I'm not following D7 development nearest than that
D5: #298180: Allow CSS Aggregation to Work with Private File Mode
D7: #298190: Allow CSS Aggregation to Work with Private File Mode
both of them marked as duplicate pointing to #146611: Allow JS/CSS aggregation in private mode
which in turns point to #166759: Public/Private File Handling
and from there to #517814: File API Stream Wrapper Conversion
Comment #61
arhak CreditAttribution: arhak commentedpatch for D6 will be exclusively maintained at #572516: make private download method support css/js aggregation, color module and js translations
Comment #64
arhak CreditAttribution: arhak commented@#62 as far as I know, none of the attached patches aims D7
Damien asked if this is present in D7 @#59, but no one has answered that
don't waste time hitting the test bot if a D7 patch doesn't exists on this thread
Comment #65
kewlguy CreditAttribution: kewlguy commented#39: 2009.09.07-public_misc03.patch queued for re-testing.
Comment #66
arhak CreditAttribution: arhak commented@#65 don't bother the bot with D6 patches (I forgot to put a
.D6.patch
suffix)I can't hide those attachments by editing the comments
whoever wants more info regarding it #572516: make private download method support css/js aggregation, color module and js translations
OTOH the issue discussed in this thread has no proposed patch for D7
patch @#6 was for color module only (don't know if aimed D6 or D7)
Comment #67
arhak CreditAttribution: arhak commentedComment #68
arhak CreditAttribution: arhak commentedtag
Comment #69
cog.rusty CreditAttribution: cog.rusty commentedThere is nothing to wait from Drupal 7. Drupal 7 has solved this kind of problems by supporting public and private downloads at the same time. If it has been decided that the feature won't be added to Drupal 6, then the issue can be set to "won't fix" and closed.
Of course the submitted patch can be useful to someone in any case.
Comment #70
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgreed with #69.
Comment #71
guidot CreditAttribution: guidot commentedUpdated patch from #38 against Drupal 6.17. Now with Unix line endings. Still some fuzz in it. Didn't brake my site yet, BUT might break your's! So use with care. Apply with
patch -p0 < css+js-aggregation-D6.17.patch
.Comment #72
arhak CreditAttribution: arhak commented@#71 that patch is being maintained at #572516-39: make private download method support css/js aggregation, color module and js translations
Comment #73
z7 CreditAttribution: z7 commentedSome time ago I created a module that does basically 2 things: allows including 3rd party libraries, like Zend Framework easily and enables dynamically generated CSS and JS while using private download mode. It is all done through Drupal hooks, so no core patch is required and should work in any environment. The only thing to consider is that you'll need some .htaccess files to disallow access to private files in the download folder.