Drush integration, cache file management, unstyled pages and code comments

Do you know what the permission situation is for clearing out Drupal core aggregated CSS files?

I would like to follow along with how that functions if possible.

I took a small look:

http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_b...

...
file_prepare_directory($csspath, FILE_CREATE_DIRECTORY);
...

They use the same permission as I do when creating the directory, but somewhere the permissions on the folder and its contents are altered.

Did you come up with any results?

The patch works fine for me.
It should be part of the dev code ASAP

I know its been a while but try the method used in the D6 version:

replace

if (!is_dir($css_path) && !file_prepare_directory($css_path, FILE_CREATE_DIRECTORY)) {

with

if (!is_dir($css_path) && !@mkdir($css_path, 0775, TRUE)) {

That should make sure that all directories have full group permissions.

I have been having these issues with my local sandboxes for quite sometime. Granted I know it's really just a matter of my own local permissions, however I did find that LESS does indeed need to use umask().

Also I was running into a weird issue:

If the less directory could not be created then the site would render... just without ANY stylesheets. I found that in the code, there was a return being called if the directory couldn't be created. However, because this is a #pre_render function, the return value needs to be the $styles array.

I am escalating this issue to major, mainly for the simple fact that the return isn't actually returning the $styles variable properly and causing the site to "break".

As far as the permissions aspect of this goes, the umask() should help relieve perhaps some people's issues. However, I would like to stress that ultimately it is the server/sandbox's responsibility to setup the permissions of the file structure properly in the first place.

Once this has been verified, could we perhaps get this committed ASAP?

Tried the patch in my build from #8 and it didn't work for me.

What's happening for me is that the /less/UNIQUE directory is created with the wrong owner/group

Re-rolled the patch from #8 to make sure the folders are created with the right permissions.

My last re-roll didn't work after more testing. Reverting to dirty hack mode

@paulsheldrake:

However, I would like to stress that ultimately it is the server/sandbox's responsibility to setup the permissions of the file structure properly in the first place.

See: Securing file permissions and ownership

The patches you submitted are indeed "hack"-ish and should not be applied to this module. Using chown() is strongly discouraged as the user:group for each server may vary. Hard-coding these changes is not the solution.

This all being said, perhaps we could implement a hook that is called before the files are created? Then perhaps we could create and include a sub-module (less_permissions) which would provide additional UI options to allow the user to set the user:group for the server? The sub-module's hook wouldn't run unless these variables were specifically set (to ensure that the proper user:group are used) and would let the server attempt to handle it (as it really should be, in the first place).

Has anyone looked into how some of the core bundled themes handle creating their custom colors.css files?

If I recall correctly they are creating files that are in the public folder, so they should also have a similar situation.

At a minimum, whats the code that creates the css folder that is used for aggregated css files?

Yes! It is actually, however, core's color module (which I'm ironically rather familiar with) that handles the stylesheet saves.

The function that actually handles this exact issue is one that I tend to forget about:_color_save_stylesheet(). It saves the file to the public folder and then calls drupal_chmod() to fix the permissions of the path in question.

I will create another patch in the AM to see if this does indeed fix the issue.

Ok, did some serious debugging and decided to play devils advocate with my permissions and here is my conclusion.

The file_prepare_directory() function actually recursively creates directories and drupal_chmod()'s them accordingly.

Where I think people have been seeing a problem is actually after the CSS file is actually created. file_unmanaged_save_data() doesn't actually drupal_chmod() the file after it's been saved. So I've updated the patch accordingly (also taking out the umask stuff).

Given the original scope of this issue (involving Drush), I would also like to point out this issue: #990812: Add a "permissions" subcommand to fix/set all file permissions. I would again like to point out that if anyone has further issues with this module and permissions it's actually probably due to incorrect server permissions (which is NOT this module or any other module's responsibility).

I think the project page should also reference Securing file permissions and ownership and this issue after it's been fixed so people stop opening up issues.

The attached patch follows core file management practices and has been re-rolled against the latest dev.

Nice catch on the return $styles. I can't believe I let that live so long.

Actually drupal_chmod does get called within file_unmanaged_save_data:

file_unmanaged_save_data calls
file_unmanaged_move which calls
file_unmanaged_copy which calls
drupal_chmod

Unless I'm missing something, the files should be created and then immediately drupal_chmod'd correctly.

Hmm, guess I missed that drupal_chmod does indeed get called (it was 1am lol). Well aside from that, I think that the only thing really to come from this patch is the minor tweaks to status message (reduce duplicate messages), properly returning the unaltered styles (this caused unstyled pages) and documentation on what's happening with the _less_new_dir() wrapper function. Attaching revised patch.

Overall my opinion still stands, if you're having issues with permissions it's probably because your server/sandbox isn't properly setup. It's not this or any other module's responsibility to fix these types of issues.

Damn, I was hoping that I was missing something.

I am incorporating the changes from #1626652-17: Drush integration, cache file management, unstyled pages and code comments.

Just a quick title change to match the final result of this issue.

Oh, I'll also be adding a link to Securing file permissions and ownership once a doc page for this module is created.

Ok, I just had yet another break though thought (sorry)!

It was still bugging me why people would be having issues with this module when things like core CSS and JS aggregation works just fine if run from drush.

I think I finally have the answer to why things are going wonky with permissions! When the CSS caches are cleared in core (drupal_clear_css_cache), all it does is remove stale files. It doesn't actually rebuild the CSS (which would invoke file_prepare_directory).

This is only done in a page deliver of some kind that actually delivers CSS files. So the files/directories are only created from a browser request (which would of course use apache's user/group when creating the files.... not drush). See: drupal_build_css_cache.

I'm gonna rework the patch one more time and see what you think :)

Alrighty, I think we have a winner!

I renamed the _less_new_dir() to _less_get_dir($reset = FALSE). This now only returns what the current less directory should be. It will generate a new unique ID if it is currently not set, empty or manually reset.

When the page is actually rendered, it will only then invoke file_prepare_directory on the less css path (which obviously includes the less directory) and recursively creates everything with the web server's permissions, not Drush's! Drush now only handles the removal of files, NOT the creation... a small little WHOO HOO!!!

I also went ahead and cleaned up the drush integration to just use less_flush_caches since it also manually resets the directory. less_flush_caches also now removes stale files, instead of just less_cron.

This indeed seems like the silver bullet for this issue.

I've implemented all the code changes and everything worked perfectly.

This will be in the next release.

Please check out the latest beta.

If you are still finding problems, please retag this issue with the new version as all future development for D7 will be on the 7.x-3.x branch.

Tagging and closing other, duplicate issue:
#1807298: Ownership Problem on FastCGI servers

Awesome glad to hear! Marking this RTBC for the 2.x branch. I know Corey already committed the changes to the 2.x branch, but there hasn't been a release yet. Should mark as fixed once that happens.

There are no current plans to have another release on the 7.x-2.x branch.

I'm re-tagging as any future test of the fix for this problem should happen on the on the current beta build of the 7.x-3.x branch.

This should be resolved in 7.x-3.0.