Problem/Motivation

The install.php script and the "Reports > Status" Report both check for the files directory to have read/write permissions, but they should be checking for read/write/execute permissions instead.

This can cause various hard-to-troubleshoot permission-related errors with the files directory, including on Docker environments.

Steps to reproduce

  • Drupal is already installed
  • (goof up permissions) chmod 766 ./sites/default/files
  • Click Reports > Status Report
  • File System is green (should be red here, directory is not executable). No big deal yet
  • Click on Configuration > Performance
  • Click either Aggregate checkboxes under Bandwidth optimization or both
  • Clear all caches
  • You'll notice the CSS problem right away if that's what you selected to aggregate

Results

The specified file temporary://file6GnL0E could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.

Proposed resolution

Install.php and Reports > Status should check for read/write/execute permissions (x77) on the directory in question

Remaining tasks

  • The patch needs to be rerolled for the latest version of Drupal 8.
  • It is not possible to add a reliable automated test for this issue.
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Instructions
Update the issue summary Instructions
Update the issue summary noting if allowed during the beta Instructions

User interface changes

  • None known.

API changes

Two functions will be added to the API:

  • file_directory_is_writable($uri)
  • drupal_is_executable($uri)

Original report by [mlehner616]

The install.php script checks for the files directory to have read/write permissions (x66), should be checking for read/write/execute instead (x77).
If the files directory is created with just read/write permissions, turning on caching/compression strips off CSS and displays errors regarding temporary files.

Install.php should check for this correctly, as well as the status report script.
Basically these two scripts don't care if execute permissions are set on ./sites/default/files

This may be a critical issue but I'll let someone more technical triage this one.
Tested multiple times and was able to reproduce this behavior:

Scenario 1

Drupal is already installed
(goof up permissions) chmod 766 ./sites/default/files
Click Reports > Status Report
File System is green (should be red here, directory is not executable). No big deal yet
Click on Configuration > Performance
Click either Aggregate checkboxes under Bandwidth optimization or both
Clear all caches
You'll notice the CSS problem right away if that's what you selected to aggregate

Here are the errors:
The specified file temporary://file6GnL0E could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileNY1O4m could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileSsnd94 could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileV3PZdN could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileSFTvjv could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileLB2jpd could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileSKgNvV could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://file1KkKCD could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileCHUVJl could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
The specified file temporary://fileNcdmR3 could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.

What's expected:
Install.php and Reports > Status Report should red flag the non-executable files directory (permissions must be 777)

Install.php actually does the same thing. It doesn't care that ./sites/default/files directory is 766 when it should be 777

CommentFileSizeAuthor
#192 944582-10.2.x-192.patch15.55 KBIliaNoz
#187 d7-944582-186-do-not-test.patch8.02 KBShaneOnABike
#185 944582-9.2.x-9.3.x-185.patch15.53 KBPinolo
#184 944582-9.2.x-9.3.x-184.patch15.52 KBPinolo
#181 944582-8.9-181.patch18.73 KBthomscode
#181 944582-8.8-181.patch17.63 KBthomscode
#178 944582-178.patch19.8 KBdas-peter
#173 944582-173.patch18.03 KBdas-peter
#172 944582-172.patch18.04 KBdas-peter
#169 944582-169.patch16.03 KBidebr
#148 interdiff-144-148.txt1.18 KBjofitz
#148 944582-148.patch16.3 KBjofitz
#144 interdiff-141-144.txt1.59 KBseanB
#144 944582-144.patch16.12 KBseanB
#141 944582-141-reroll-8.5-8.6.patch16.08 KBseanB
#139 944582-139-add-executable-check.patch15.91 KBsolotandem
#137 944582-137-add-executable-check.patch15.9 KBsolotandem
#135 944582-135-add-executable-check.patch15.85 KBsolotandem
#133 944582-133-interdiff.txt2.37 KBsolotandem
#133 944582-133-add-executable-check.patch15.83 KBsolotandem
#130 944582-130.patch14.54 KBruudvanoijen
#129 8-5-dev-944582-128.patch13.73 KBruudvanoijen
#129 8-4-dev-944582-128.patch14.54 KBruudvanoijen
#124 944582-124.patch15.35 KBjofitz
#123 sites_default_files-944582-122.patch15.56 KBloicLEMEUT
#120 sites_default_files-944582-120.patch15.36 KBFonski
#115 sites_default_files-944582-115.patch15.34 KBjproctor
#110 sites_default_files-944582-110.patch12.1 KBmvc
#102 sites_default_files-944582-102.patch9.19 KBalvar0hurtad0
#100 944582-is_exec_writ-100-8.0_0.patch9.93 KBankitgarg
#98 is_exec_writ-944582-98.patch9.93 KBadci_contributor
#95 944582-is_exec_writ-95-8.0_0.patch10.32 KBmgifford
#93 944582-is_exec_writ-92-8.0.patch10.4 KBjohnish
#91 944582-is_exec_writ-91-8.0.patch10.43 KBsuperspring
#70 issue_944582-drupal-7.18.patch8.03 KBorlitzky
#59 d7-944582-59-do-not-test.patch8.1 KBxjm
#59 d7.12-944582-59-do-not-test.patch8.03 KBxjm
#56 directory-executable-944582-56.patch8.36 KBDavid_Rothstein
#40 Screen Shot 2012-02-15 at 1.22.50 PM.png53.13 KBryan.gibson
#31 directory-executable-944582-31.patch8.57 KBkathyh
#15 file_error_update.jpg61.05 KBxaav
#12 directory-executable-944582-12.patch9.54 KBDavid_Rothstein
#11 directory-executable-944582-11.patch8.94 KBDavid_Rothstein
#9 directory-executable-944582-9.patch6.08 KBDavid_Rothstein
Capture.PNG60.88 KBmlehner616
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anavarre’s picture

Version: 7.0-beta1 » 7.0-beta2

I have the same problem in 7.0-beta 2 and was forced to manually delete both the js and css directories within files to make the errors go even though I unchecked CSS/JS minification.

mlehner616’s picture

I see this as a major issue. It's not a show stopper but thinking of the less tech savvy users of Drupal perhaps we should bump it up. We don't seem to be getting the attention of the developers here.

anavarre’s picture

Version: 7.0-beta2 » 7.0-beta3
Status: Active » Needs review

Now with 7.0-beta3, everything seems fine.

zerodeux’s picture

Testing files access and ownership within PHP will never work correctly. You can't emulate the filesystem logic for all OS, ACL variant and whatever NFS strangeties. It just failed on a very simple case for me 10min ago.

If you need to know if a file can be written to, you fopen($filename, "r+") and check the result. Then use the handle or close it.

Besides it's racy (and thus risky) to check file permission, then manipulate the file. Countless security issues arise from this misconception.

Fixing this is quite complicated since drupal_verify_install_file() check if files, dirs or symlinks can be read, written or executed. Callers should actually *try* to do what they're only checking (read, write, exec), and then handling exceptions as usual.

And this function heavily calls drupal_install_fix_file() which means that drupal_verify_... does modify files. It tries to fix things, and user-friendlines is part of Drupal experience, so let's be it. But it will pass a given test if drupal_install_fix_file() succeeds *without* re-running the test. On a NFS setup it's possible for a chmod() to succeed without having the intended effect on bitmask (because the NFS server logic decides and it sometimes looks funny to a client). This:

if (!is_writable($file) && !drupal_install_fix_file($file, $mask))  { return FALSE; }

Should be (ugly, yes):

if (is_writable($file)) { return TRUE; }
elsif (drupal_install_fix_file($file, $mask) && is_writable($file)) { return TRUE; }
else { return FALSE; }

I see 12 calls of drupal_verify_install_file() in Core. Worth patching ? It's maybe not a security risk, although this design is flaky.

mlehner616’s picture

What about PHP's fileperms function? I'm not a familiar yet with the Drupal API but it seems that PHP's implementation of fileperms seems to work MOST of the time and should certainly work on a no frills CentOS 5.5 server which is the OS I'm having issues with.

substr(decoct( fileperms('.') ), 2); // To get '777'
substr(decoct( fileperms('.') ), 1); // To get '0777'

right?

mlehner616’s picture

lvbeck’s picture

same issue with 7.0-rc1

David_Rothstein’s picture

Version: 7.0-beta3 » 7.x-dev
Status: Needs review » Active

This should be "active", not "needs review", since there is no patch yet. It's probably not hard to write one, though :)

Let's keep this focused on the original bug report. Some of the last few comments seem like they're about something different.

David_Rothstein’s picture

Component: install system » file system
Status: Active » Needs review
FileSize
6.08 KB

Here's a patch. I've only tested it lightly, though.

It seems there are a number of places where we check these permissions on directories. The issue really doesn't have a whole lot to do with the installer itself, so I'm moving it to the "file system" component also.

Status: Needs review » Needs work

The last submitted patch, directory-executable-944582-9.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
8.94 KB

Apparently is_executable() is one of the PHP functions that doesn't always work well with stream wrapper URIs - that is causing many (if not all) of the failing tests.

Let's try it with a Drupal wrapper for that function instead, similar to some of the other replacement functions in file.inc.

David_Rothstein’s picture

OK, so the tests pass. I guess it also makes sense to create a helper function for the combined writable/executable check, rather than repeating it everywhere. The attached patch does that.

mlehner616’s picture

mgifford’s picture

That patch applied well to my last RC. Still not sure if that works through the permissions problem on the site I've been trying to upgrade mind you.

xaav’s picture

FileSize
61.05 KB

I'm also getting this error.
See attached screenshot.

Christopher James Francis Rodgers’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -Needs issue summary update, -Needs backport to D7

2011.06.07
This solution relates strictly to the Drupal 7 error...

===
Error:
The specified file temporary://file****** could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
===

The Solution for me was simply..
.
Go to 'Configuration' > 'Media' > 'File System'
[*root*]/admin/config/media/file-system
and change the 'Temporary directory' from...
/tmp
...to...
tmp

(Do Not use a "/" to begin the directory specification)

See also http://drupal.org/node/1106492#comment-4562780
for all of my known details of that error.

hbblogger’s picture

I have the same problem. I thought that it had to do with my server as drupal 7 works fine on WAMP, but I see that I am not alone! If I copy the file using Filezilla, all is well but if I have a file that needs to write something to a directory (such as an image) the problem returns. I get the same error as geoffreyfishing. I have tried changing group permissions to 'write' in my site and include directories but it has had no effect. I get an error when I try to change the name of the tmp file (basically saying that it is unable to create the file). I see that David produced some code presumably to fix the problem, but I am not sure what to do with this. This is a serious problem as it makes drupal very difficult to use.

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Christopher James Francis Rodgers’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Moved: Consolidated into comment #16 above.

http://drupal.org/node/944582#comment-4231504

NotNotCow’s picture

#16 My god,

All afternoon trying to figure it out and that's the flippin solution!

[smacks forehead]

Thanks for posting!

NNC

rickmanelius’s picture

When trying to use development version of devel to generate content in Drupal 7, I get this same error. #16 doesn't solve. So hopefully the backported patch corrects this!

rickmanelius’s picture

Solved my problem.
So when you specify a file or image field in a content type, the directory isn't necessarily available already. If you try to run devel generate to make a batch of content, the lack of this destination folder throws the same type of error as original post above.

If you manually create a single node and enter the information for these fields, then devel can add additional items without a problem. So there is some disconnect with the field folders. Perhaps they should be created on the content type configuration save. But that still may lead to issues if something tries to access it programmatically before that.

a_webb’s picture

I get the #19 bolded error code, and cannot seem to fix. i am upgrading from 7.2 to 7.4. i tried #16 solution to no avail. my file.inc specifies the /sites/default/files/tmp folder, which is there and has 777 permissions. There is no upload_tmp_directory set in my php.ini file (as that setting is commented out.) I am at a total loss as everything else with my upgrade went fine. This is a big site for a college, so stuff like this *kills* productivity. Kudos to anyone who can help, I'll return the favor sometime!

gaurav26570’s picture

For me Solution #16 worked..Thanks.

xjm’s picture

Tagging issues not yet using summary template.

maxchock’s picture

I'm using Panels and this error cause all my page layout with Panel doesn't work.. error message from log:

TYPE	file
DATE	Saturday, July 30, 2011 - 16:14
USER	max
LOCATION	http://www.trekker.my/node/2
REFERRER	http://www.trekker.my/node/2
MESSAGE	The specified file temporary://filecAw4AM could not be copied to public://ctools/css/4314fe7864ddd4cd7ddcc2cd7b74323f.css.
SEVERITY	error
HOSTNAME	175.138.122.218
OPERATIONS	

my tmp folder work fine, but the "ctools/css/" just empty...

Fannon’s picture

Got this error with the latest 7.7 Version after an upgrade from 6.x

Solved this with giving 777 Permissions on several folders. But which ones need it really?

Greets,
Simon

RowboTony’s picture

I had this problem too after moving my local development site to be live on a shared hosting environment - #16 fix worked partially - I was still getting errors on several other pages such as admin/appearance. Looking at the recent log entries, admin/reports/dblog, I found that this was due to Drupal wanting to recreate it's own cache directories. For example - the log entry showed the error:

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

I deleted the css directory at sites/default/files/css, then Drupal would happily recreate the css directory with the proper owner and permissions. This was also true for the directory sites/default/files/js. Once Drupal recreated those all of the errors were gone and all was right with the world.

nikkubhai’s picture

I am facing same problem. Changing /tmp to tmp does not work!

xjm’s picture

Status: Needs review » Needs work
Issue tags: -drupal installation issue, -file permissions +Novice

This needs to be rerolled for both #22336: Move all core Drupal files under a /core folder to improve usability and upgrades and the git migration. Tagging as novice for the task of rerolling the patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

kathyh’s picture

update for #30 - d8 /core and git. change this to needs work after testbot for other issues/comments. carry on.

andypost’s picture

I think this related and should be included #484554-7: Stop relying on Apache for determining the current path

mlehner616’s picture

mesr01’s picture

@nikkubhai (#29), you may try to manually create a subfolder inside /tmp, let's say /tmp/d7/subdomain.example.com (replacing last part with actual site folder name for recognition purpose, although any subfolder name would be equivalent). Set permissions to 777 for all three folders and change the setting in admin/config/media/file-system accordingly.

I noticed that writing directly to /tmp, even with permissions set to 777, won't work with some D7 installs. I don't know at this point if this is a problem with D7 or a hosting account feature.

TransitionKojo’s picture

Issue summary: View changes

Updated summary to comply with Issue Summary Standards

TransitionKojo’s picture

Left out my username: Transition (with help from XJM)

TransitionKojo’s picture

Issue summary: View changes

added "code" tags around error message in results section; User: Transition

TransitionKojo’s picture

Issue summary: View changes

Updated "API changes" section to reflect 2 API Additions; user: Transition

TransitionKojo’s picture

Issue summary: View changes

added "related issues" sub-section in "Remaining Tasks" section; user=Transition

TransitionKojo’s picture

Issue summary: View changes

Updated "Remaining Tasks" section; user=Transition

TransitionKojo’s picture

Issue summary: View changes

Corrected link to Item #31; User=Transition

TransitionKojo’s picture

Issue summary: View changes

fixed typo in "

  • Patch in #944582-31: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows) implements the proposed solution and passes automated tests.
  • "; user=Transition

    xjm’s picture

    Summary added by Transition.

    jagalbraith’s picture

    Version: 8.x-dev » 7.10

    Guys this is an easy fix. It is only a Unix files permission problem. Here is the fix:

    1. Change directory to the root of your Drupal site.

    cd /var/www

    2. Set your permissions to 777 and try it out. You can back off permissions to lock it down later, but get it working for now. Ensure to do it RECURSIVE. the -R is what is killing people in this post because there are already files in the directory that need to be writable for Drupal to work properly.

    chmod 777 -R sites/default/files

    3. Now create a directory called tmp at the root of your site.
    mkdir tmp

    4. Set the permissions there too.
    chmod 777 -R tmp

    Enjoy.

    John Galbraith
    www.binarybb.info

    Tor Arne Thune’s picture

    Version: 7.10 » 8.x-dev
    xjm’s picture

    Issue tags: +Needs manual testing

    Tagging for manual testing.

    xjm’s picture

    Issue summary: View changes

    Updated issue summary.

    ryan.gibson’s picture

    Assigned: Unassigned » ryan.gibson

    I will test it.

    ryan.gibson’s picture

    I went through the steps and was able to reproduce the issue.

    I then tested the patch and upon clearing the cache received a warning message that there was something wrong with my Drupal installation.

    After navigating the to startus report page, I had the appropriate error. (See screenshot)

    message

    So, the patch looks good :D

    Let me know if I missed something or need to do something else.

    Thanks all! And thanks xjm for your continued help and support.

    ryan.gibson’s picture

    Issue summary: View changes

    Updated issue summary.

    xjm’s picture

    Status: Needs review » Reviewed & tested by the community
    Issue tags: -Novice, -Needs manual testing, -status report

    Alright, based on @ryanissamson's testing, I think this is RTBC. I also updated the issue summary. (It's not possibly to test this reliably with an automated test, as indicated in the summary.) Thanks everyone!

    xjm’s picture

    Assigned: ryan.gibson » Unassigned
    catch’s picture

    Status: Reviewed & tested by the community » Needs work
    +++ b/core/includes/file.incundefined
    @@ -2108,6 +2108,23 @@ function file_get_mimetype($uri, $mapping = NULL) {
     /**
    + * Determines if a directory is writable by the web server.
    + *
    + * In order to be able to write files within the directory, the directory
    + * itself must be writable, and it must also have the executable bit set. This
    + * helper function checks both at the same time.
    + *
    + * @param $uri
    + *   A URI or pathname pointing to the directory that will be checked.
    + *
    + * @return
    + *   TRUE if the directory is writable and executable; FALSE otherwise.
    + */
    +function file_directory_is_writable($uri) {
    +  return is_writable($uri) && drupal_is_executable($uri);
    +}
    

    drupal_realpath() doesn't work for remote stream wrappers. That's now documented for drupal_realpath() and it's even marked as deprecated.

    So if we have no choice other than to use it, we need a similar comment here, so no-one ever tries to use this for remote stream wrappers. Same for the other helper functions added. To an extent I'd consider actually dropping the helpers here and just inlining these checks, since we actively want to discourage people from using these in 99% of cases.

    12 days to next Drupal core point release.

    xjm’s picture

    Okay I was totally confused by #43 until I realized it actually refers to this function (not the one quoted above):

    +++ b/core/includes/file.incundefined
    @@ -2261,6 +2278,27 @@ function drupal_dirname($uri) {
    + * Determines if a file or directory is executable.
    + *
    + * PHP's is_executable() does not fully support stream wrappers, so this
    + * function fills that gap.
    + *
    + * @param $uri
    + *   A URI or pathname pointing to the file or directory that will be checked.
    + *
    + * @return
    + *   TRUE if the file or directory is executable; FALSE otherwise.
    + *
    + * @see is_executable()
    + * @ingroup php_wrappers
    + */
    +function drupal_is_executable($uri) {
    +  // By converting the URI to a normal path using drupal_realpath(), we can
    +  // correctly handle both stream wrappers and normal paths.
    +  return is_executable(drupal_realpath($uri));
    +}
    

    I read the doc for drupal_realpath(); unfortunately, it doesn't seem to indicate what one is supposed to use instead:

    This function was originally written to ease the conversion of 6.x code to use 7.x stream wrappers. However, it assumes that every URI may be resolved to an absolute local filesystem path, and this assumption fails when stream wrappers are used to support remote file storage. Remote stream wrappers may implement the realpath method by always returning FALSE. The use of drupal_realpath() is discouraged, and is slowly being removed from core functions where possible.

    Only use this function if you know that the stream wrapper in the URI uses the local file system, and you need to pass an absolute path to a function that is incompatible with stream URIs.

    wooody’s picture

    Hi , i copy my site to new domain , and i made database and import the sql file.

    everything work fine , but i can't install any new module.

    this message

        The specified file temporary://file3x433g could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
        http://ftp.drupal.org/files/projects/xmlsitemap-7.x-2.0-rc1.zip could not be saved to temporary://update-cache-a3b7b25e/xmlsitemap-7.x-2.0-rc1.zip.
        Unable to retrieve Drupal project from http://ftp.drupal.org/files/projects/xmlsitemap-7.x-2.0-rc1.zip.
    

    And in Status report i saw file system green
    File system Writable (public download method)

    Any help.

    catch’s picture

    OnkelTem’s picture

    Drupal doesn't support ACLs.
    One solution was provided here 5 months ago and didn't get enough feedback. It is marked 'duplicate' by @catch for now. No other solution I seen, and it seems that nobody actually using ACLs. Ok, I don't care anymore about this.

    orlitzky’s picture

    The patch here doesn't fix #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs. PHP's is_writable() returns false when the directory is writable, so the file_directory_is_writable() function introduced here still fails since it calls is_writable().

    The patch that OnkelTem posted in #1333390 does fix it; and as a side effect, fixes this, because it actually writes the file. You can't do that if the directory isn't executable. (See the comments in #4)

    This is rather important to me, as I'm attempting to migrate a large number of Drupal sites to a server that uses ACLs. I'll have to manually patch every install after every update until this is fixed.

    paularmand’s picture

    Agreeing on #48: I also have ACLS set on the files directory, and this is not a marginal phenomenon. I agree with mjorlitzky: testing by writing to file (and handling errors in the test) is at this moment our best possible option.

    xjm’s picture

    Issue tags: +Needs manual testing

    I re-closed #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs as a duplicate. Let's broaden the scope of this issue to include that bug, and update the patch here to use the technique from that issue:

    The patch at #944582 fixes the executability test by introducing a wrapper around is_executable() called drupal_is_executable(). However, it also adds a wrapper, file_directory_is_writable(), around is_writable(), which calls both is_writable() and the new drupal_is_executable().

    The latter is incorrect; the only writability test that has any chance of working is the one in this issue: you write a file. If we replace the file_directory_is_writable() in that issue with OnkelTem's implementation of drupal_is_writable() from this issue, it should fix both.

    This is the patch we should incorporate into #31:
    http://drupal.org/files/linux-acl-drupal-is-writable-d7.9.patch

    We also still need to address catch's feedback from #43.

    orlitzky’s picture

    Thanks, this seems like the most sensible solution to me.

    OnkelTem’s picture

    agreed

    David_Rothstein’s picture

    I don't think I understand what's going on here but maybe my brain is fuzzy.

    The issue summary in #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs says that the only problem with ACLs comes when is_writable() is called on a stream. My patch above dealt with a similar issue for is_executable(); the solution I tried was to convert to a non-stream using drupal_realpath().

    @catch pointed out in #43 that this function is deprecated (because it doesn't work on remote stream wrappers), but (as he also pointed out) that doesn't mean we can't use it or something like it here... we just have to be careful to make sure we're only using it on local files (which I believe is the case), and document what we're doing rather than trying to make generically reusable functions.

    So if we do what @catch suggested, and additionally do it for is_writable() - rather than just for is_executable() - won't that solve all the problems at once, in a relatively simple way?

    I really am not excited about any solution that resorts to writing "test" files to determine writability. I believe we do that in one place in Drupal currently (when writing settings.php in the installer) but that's a very special case, and the resulting code to deal with all the edge cases involved with that is a bit of a mess. I would not like to start doing that everwhere in Drupal except as an absolute last resort.

    OnkelTem’s picture

    @David_Rothstein

    Would you clarify your proposition via providing alternative patch which we can test on ACL-enabled setups?

    orlitzky’s picture

    The currently writability test just looks at some filesystem bits. This does not take into account,

    • Filesystem ACLs
    • SELinux
    • Mount options

    Therefore it does not actually test writability. I agree that writing a test file is ugly, but there's no better way to do it (if someone can think of one, by all means, implement it).

    David_Rothstein’s picture

    Status: Needs work » Needs review
    FileSize
    8.36 KB

    Sure, here's a patch along the lines of what I had in mind.

    It would need more work to be a real patch, but I think this is probably good enough to test. The essential change is from this:

    +  return is_writable($uri) && drupal_is_executable($uri);
    

    to this:

    +  return is_writable(drupal_realpath($uri)) && drupal_is_executable($uri);
    

    ***

    @mjorlitzky: So is the issue summary at #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs incorrect, then? In any case, seems like this is worth testing.

    orlitzky’s picture

    I'm not even sure anymore...

    All of our sites where this is a real issue are running 7.12 and the patch won't apply that far back. For what it's worth, I set up an 8.x-dev installation on my local machine and IMCE (which calls file_prepare_directory) seems to work with the patch.

    xjm’s picture

    @mjorlitzky, if the patch is accepted, it will also be backported to Drupal 7. We can probably create a "draft" D7 version for testing as well.

    xjm’s picture

    Like these. The first applies to 7.x HEAD, the second to 7.12.

    orlitzky’s picture

    Thank you, I'll try to move a few sites to our new server later tonight and they'll need to be patched. I'll let you know how it goes.

    orlitzky’s picture

    Well, I've moved a 7.12 site, patched, and the issue with IMCE is fixed. Thanks!

    OnkelTem’s picture

    I confirm that patch works on clean Drupal 7.12 on ACL-enabled setup.
    I haven't tested on remote file-systems though, which is actually not very important in most setups.

    Both approaches - test-writing and with using drupal_realpath() have some drawbacks, but I personally like this solution more, despite this warning in docs:

    The use of drupal_realpath() is discouraged, and is slowly being removed from core functions where possible.

    I hope we will find another solution until drupal_realpath() is removed from core.

    xjm, David_Rothstein - thank you!

    maxterner23’s picture

    I got the same problem and resolved by making small modification as #36 and #28 suggested in the comment.

    chmod 777 -f sites/default/files/js

    chmod 777 -f sites/default/files/css

    and it resolves the issue

    anarcat’s picture

    Status: Needs review » Reviewed & tested by the community
    Issue tags: +DrupalWTF

    I was able to apply http://drupal.org/files/d7-944582-59-do-not-test.patch to Drupal 7.15 and it works.

    I think this is a major WTF for users on shared hosting that use ACLs. Quite hard to diagnose too!

    I wonder if D6 is affected... probably not, because it doesn't use stream wrappers.

    webchick’s picture

    Status: Reviewed & tested by the community » Needs work

    Hm. I tried to duplicate this error using the (well-written, thanks!) reproduction instructions from the OP in 8.x and was unable to do so. :( Upon submitting the performance form, I got just a simple "The configuration options have been saved." message.

    Also, the patch in #56 no longer applies, so needs work for that.

    Could someone please provide instructions on how to cause this error on D8? Or is it a D7 only issue?

    OnkelTem’s picture

    @anarcat

    Yes, D6 is affected. At least I remember for sure that I have been creating ugly 'write-test' patch for it.

    wrg20’s picture

    I recently installed upgrades/security fixes for feeds, workbench moderation, pathologic and fontyourface. I got the same error "The specified file temporary://******** could not be copied...". My status report looked fine.

    I fixed the problem by implementing #36 and #63.

    orlitzky’s picture

    I tried installing D8 but I get a symfony error, so... unknown.

    The patch in #59 still works for D7, can we have that reviewed/tested? The issue that we ran into was that if an ACL allowed read/write access to the (otherwise unreadable) sites/default/files directory, IMCE would claim that it couldn't write there and fail.

    orlitzky’s picture

    PIng. Another release I'm patching manually to get Drupal to work at all with ACLs.

    orlitzky’s picture

    And again. Here's a patch rebased for 7.18.

    orlitzky’s picture

    And again. The patch from comment #70 still applies cleanly to 7.19.

    YesCT’s picture

    Version: 8.x-dev » 7.x-dev
    Status: Needs work » Needs review
    Gábor Hojtsy’s picture

    Yes, in fact #1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed) in fact deal with *removing* the directory executability check because it was bugos on Windows. Looks like these two issues contradict each other?

    YesCT’s picture

    Yes. In those issues, we used the rational that the files dir was not tested for being executable for a reason to take out the executable checking. Another thought there was that trouble with a non-executable directory was so rare, checking was not needed.

    So, I'm wondering if people who are familiar with this issue, can provide some guidance and an update:

    1) How common is the problem with D7 (or D6) that someone installing or running a site will have incorrect permissions that need to be fixed? Here and in those other issues, to test, the execute permission was deliberately removed. Aside from that, does the situation happen?

    2) checkout the approach in #1886164: Install in different language requirement warns translations dir not executable, when the dir is executable on some systems. That tries to keep the checks for executable in, but make it work for windows by checking if whatever/. is readable since the is_executable() function does not work for all systems. How is that different from the approach here?

    orlitzky’s picture

    Another ticket, #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs, was merged into this one and the patch here was made to solve both problems. Half of that solution somewhat conflicts with #1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed), it seems, and the other half (the ACL fixes) should not.

    One half of the fix, from #1333390, just makes the writable/executable check work in the presence of ACLs. It does this by adding new functions file_directory_is_writable and drupal_is_executable. This has nothing to do with #1885510 and should be fine to apply anyway.

    The other half from this ticket adds the executable check to the installer, and may conflict with #1885510. This is visible in the patch by the change,

    diff --git a/includes/install.core.inc b/includes/install.core.inc
    index 9805e1c..854c597 100644
    --- a/includes/install.core.inc
    +++ b/includes/install.core.inc
    @@ -1592,7 +1592,7 @@ function install_check_requirements($install_state) {
         // Otherwise, if settings.php does not exist yet, we can try to copy
         // default.settings.php to create it.
         elseif (!$exists) {
    -      $copied = drupal_verify_install_file($conf_path, FILE_EXIST|FILE_WRITABLE, 'dir') && @copy($default_settings_file, $settings_file);
    +      $copied = drupal_verify_install_file($conf_path, FILE_EXIST|FILE_WRITABLE|FILE_EXECUTABLE, 'dir') && @copy($default_settings_file, $settings_file);
           if ($copied) {
             // If the new settings file has the same owner as default.settings.php,
             // this means default.settings.php is owned by the webserver user.
    

    I personally would be happy without the executable check at install time. My primary concern is that the unpatched writable/executable checks fail in the presence of ACLs. This breaks IMCE, and all of our websites.

    Gábor Hojtsy’s picture

    Status: Needs review » Needs work
    Issue tags: +D8MI

    #1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed) got committed, so that would need to be taken into account for any further patch updates here

    orlitzky’s picture

    If you want the "executable" check to work on both Windows and sane operating systems, you need to forget the is_executable() nonsense, and create e.g. drupal_is_traversable() instead.

    On Windows, that can just return true, or delete My Photos, or whatever -- I don't care. Everywhere else it should do the drupal_is_executable() check.

    In any case, it was clearly a mistake to merge these two issues. Can we please create a separate issue for fixing the existing permissions checks which make drupal unusable? This has been a huge waste of time for something so basic as "can I create a file in this directory".

    David_Rothstein’s picture

    Well, there's no reason to make the installer inconsistent with everything else, so that doesn't seem like a meaningful way to split things to me. But if you think #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs can be solved without running into the issues here you should consider reopening that one.

    As far as I can tell, though, this is all pretty crazy, and #1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed) at most just worked around the problem? If it's really true that Windows will fail if the directories are (forced to be) executable and Linux will fail if they aren't, that is a tough one to solve :) I certainly didn't realize that before.

    orlitzky’s picture

    I just created #1899126: [D7] Add wrappers to fix permission checks with a new patch that doesn't touch the installer.

    We don't really care if the directory is executable, we care that its traversable. On everything else, you just check the execute bit. On Windows, that doesn't work -- so one possible fix is to just traverse the damn thing and see if it works. There's a note about this in the PHP is_executable() documentation.

    star-szr’s picture

    Version: 7.x-dev » 8.x-dev

    This should go back to 8.x at least…

    YesCT’s picture

    adding issue summary initiative tag to encorporate what we have found out about the related issues (wrapper for permission check, can't install on windows) and to try and write out some remaining tasks and next steps here.

    kurt.iverson’s picture

    This worked for me. Thanks!

    Blooniverse’s picture

    Since #1987262: [Installation Error] Symfony: "The service definition 'request' does not exist." is related: any recommendations for us there? Any input how to proceed best?

    jair’s picture

    Issue tags: +Needs reroll
    pratik60’s picture

    On changing sudo chmod 766 ./sites/default/files, I get a white screen of death on 'admin/reports'.

    On checking the logs, I got this

    'PHP Fatal error: Call to undefined function user_access() in /var/www/d8/core/includes/menu.inc on line 599'

    On further checking, I found on https://drupal.org/node/2049309, that user_access() got converted to a method on the user/accountInterface. On the page, the description states that '
    The user_access() method was directly replaced by AccountInterface::hasPermission()'

    Hence the error!

    pratik60’s picture

    Issue summary: View changes

    Updated issue summary.

    mkapst’s picture

    #16 worked for me. Cheers to Christopher James Francis Rodgers!

    XTCHost’s picture

    I just fixed my ./sites/default/files directory permission with from the site domain or default folder, whichever you use

    chown -R user:user files

    user being the CPanel username on linux server

    arkestra’s picture

    Status: Needs work » Needs review

    70: issue_944582-drupal-7.18.patch queued for re-testing.

    Status: Needs review » Needs work

    The last submitted patch, 70: issue_944582-drupal-7.18.patch, failed testing.

    The last submitted patch, 70: issue_944582-drupal-7.18.patch, failed testing.

    superspring’s picture

    Status: Needs work » Needs review
    FileSize
    10.43 KB

    Rerolled

    johnish’s picture

    Assigned: Unassigned » johnish

    I'm going to try and reroll this.

    johnish’s picture

    Issue tags: -Needs reroll
    FileSize
    10.4 KB

    I rerolled this.

    johnish’s picture

    Assigned: johnish » Unassigned
    mgifford’s picture

    Just a reroll...

    YesCT’s picture

    Issue summary: View changes
    Issue tags: +Needs reroll
    YesCT’s picture

    Status: Needs review » Needs work
    adci_contributor’s picture

    Status: Needs work » Needs review
    FileSize
    9.93 KB

    Trying to reroll

    Status: Needs review » Needs work

    The last submitted patch, 98: is_exec_writ-944582-98.patch, failed testing.

    ankitgarg’s picture

    Status: Needs work » Needs review
    FileSize
    9.93 KB

    Rerolled #95.

    Status: Needs review » Needs work

    The last submitted patch, 100: 944582-is_exec_writ-100-8.0_0.patch, failed testing.

    alvar0hurtad0’s picture

    YesCT’s picture

    Status: Needs work » Needs review

    changing status to needs review so the testbot will run on the patch.

    Status: Needs review » Needs work

    The last submitted patch, 102: sites_default_files-944582-102.patch, failed testing.

    pbuyle’s picture

    Coming from https://www.drupal.org/node/2018947

    +++ b/core/includes/file.inc
    @@ -431,6 +431,46 @@ function file_prepare_directory(&$directory, $options = FILE_MODIFY_PERMISSIONS)
    + * Determines if a directory is writable by the web server.
    + *
    + * In order to be able to write files within the directory, the directory
    + * itself must be writable, and it must also have the executable bit set. This
    + * helper function checks both at the same time.
    + *
    + * @param $uri
    + *   A URI or pathname pointing to the directory that will be checked.
    + *
    + * @return
    + *   TRUE if the directory is writable and executable; FALSE otherwise.
    + */
    +function file_directory_is_writable($uri) {
    +  // By converting the URI to a normal path using drupal_realpath(), we can
    +  // correctly handle both stream wrappers and normal paths.
    +  return is_writable(drupal_realpath($uri)) && drupal_is_executable($uri);
    +}
    +
    

    This would not properly support remote filesystem exposed as stream (eg. Amazon S3) as drupal_realpath() only support local URI.

    The proper way would be something like

    function file_directory_is_writable($uri) {
      // By converting the URI to a normal path using drupal_realpath(), we can
      // correctly handle both stream wrappers and normal paths.
      $realpath =  drupal_realpath($uri);
      return is_writable($realpath ? $realpath : $uri) && drupal_is_executable($uri);
    }
    
    pbuyle’s picture

    A bit more information on the issue. Everything was working fine when we were using NFS v3, we started to have issue once switching to NFS v4.

    When using NFS v4, all the files are shown as owned by "nobody:nogroup" with the rwx for both the user and the group, and r-x for others. But all users are actually able to write all files.

    Using Drupal 7.42, we can observe different results when using is_writable() on a local file path vs. the path to the same file using the public:// stream wrappe: is_writable(drupal_realpath('public://styles')) returns TRUE, while is_writable('public://styles') returns FALSE.

    Using the patch in #70 fixes to issue for us (but with the same remote file system issue as reported in #105.

    OnkelTem’s picture

    For those still seeking solution for Drupal 7 — patch from #70 (for 7.18) still applies fine on 7.43.
    Without it Linux ACL won't work.

    Version: 8.0.x-dev » 8.1.x-dev

    Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

    Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    serundeputy’s picture

    Issue summary: View changes
    mvc’s picture

    Status: Needs work » Needs review
    FileSize
    12.1 KB

    Reroll of #102 for 8.2.x-dev, plus added the wrapper to a few missing instances of is_writable() and is_executable().

    @pbuyle I didn't extend this to cover Amazon S3 since I need the patch only for NFSv4 & extended ACL's but wrapping all calls with file_directory_is_writable() should at least make fixing that issue simpler.

    Status: Needs review » Needs work

    The last submitted patch, 110: sites_default_files-944582-110.patch, failed testing.

    Version: 8.1.x-dev » 8.2.x-dev

    Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

    Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    rkcreation’s picture

    Hi

    #70 works for me (Drupal 7.51).

    I had a weird issue : drupal in a docker container (derived from official php container) doesn't let me upload files with the "cck" file field. However, simple form API' file field works well (eg. in appearance > settings > logo field).
    After investigating, I found that was php is_writable which returned a false false in file_prepare_directory (includes/file.inc), while my directory was in fact writable : if I bypass is_writable check, upload works like a charm :-) even in the case of the directory doesn't exist. I think it's related to the docker mounted volume and the way php handles uri in is_writable, but with drupal_realpath, is_writable' return is correct.

    Could we hope a patch in a future 7.x release ?

    Thanks anyway for the patch !

    FrenchBarbu’s picture

    Hi

    Got the exact same issue on D8 and Docker : is_writable and URI return false. If I comment this part everything is ok so the directory is writable. Any clues ?

    jproctor’s picture

    Status: Needs work » Needs review
    FileSize
    15.34 KB

    Reroll of #110 for current 8.2.x-dev, plus added the wrapper to a couple more occurrences of is_writable().

    This doesn't solve the similar-but-unrelated problem I was having and it fails testing for me, but my rig has other problems so I figured I'd at least get the patch applying cleanly in case there's value in moving it forward.

    Status: Needs review » Needs work

    The last submitted patch, 115: sites_default_files-944582-115.patch, failed testing.

    Version: 8.2.x-dev » 8.3.x-dev

    Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

    Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    acidrobot’s picture

    I'm using linux NFSv4 ACLs and this was a real headache!
    Using the patch #70 works for me.
    I also verified when removing the patch that I was unable to upload files to sites/default/files.
    I'm quite surprised that Drupal isn't supporting ACLs inherently.
    IMHO chmod 777 to ANY file | folder is never a secure solution, and I would never implement it on a production environment.

    TL;DR::
    Patch #70 Fixes issues with uploading files & permissions with Linux ACLs.

    B-Prod’s picture

    Tested #115: it fixes the issue.

    Context: Docker image with mounted volume.

    Without this patch, the directories inside "public://", including this last itself, are stated as non writeable even if they are.

    Fonski’s picture

    Tried to reroll the patch for 8.3

    jeremylichtman’s picture

    I've tested out patches #115 and #120. Doesn't full resolve the issue, because the function file_unmanaged_save_data (circa line 1005 in web\core\includes\file.inc) calls the php function file_put_contents, which fails for all of the reasons noted above.

    Version: 8.3.x-dev » 8.4.x-dev

    Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

    Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    loicLEMEUT’s picture

    Hi,

    Here is my patch for Drupal 8.3.7, patch #120 doesnt work with this Drupal version.

    Thanks

    jofitz’s picture

    Status: Needs work » Needs review
    FileSize
    15.35 KB

    Re-rolled.

    Status: Needs review » Needs work

    The last submitted patch, 124: 944582-124.patch, failed testing. View results

    a.milkovsky’s picture

    The latest patch works for Drupal 8.4 as well.

    a.milkovsky’s picture

    The Jenkins error looks like this:

    PHP Fatal error: Uncaught Error: Call to undefined function Drupal\Component\FileSystem\file_directory_is_writable() in /var/www/html/core/lib/Drupal/Component/FileSystem/FileSystem.php:37

    I looks like legacy include (\Drupal\Core\DrupalKernel::loadLegacyIncludes) did not work.
    We should include the file.inc here or use a service.

    ptmkenny’s picture

    Version: 8.4.x-dev » 8.5.x-dev
    Issue summary: View changes

    Updated issue summary.

    ruudvanoijen’s picture

    This patch doesn't apply anymore because of issue: 2880445 In this issue config.install is removed. So I Rerolled a patch for 8.4-dev

    I don't know why 8.5-dev is missing the logic for a test in HtmlOutputPrinter. Removed this in the patch for 8.5-dev.

    ruudvanoijen’s picture

    New patch for 8.4.

    jonathanshaw’s picture

    Issue tags: +Needs tests

    The patch uses drupal_realpath(). @Catch points out in #43 that

    drupal_realpath() doesn't work for remote stream wrappers. That's now documented for drupal_realpath() and it's even marked as deprecated. So if we have no choice other than to use it, we need a similar comment here, so no-one ever tries to use this for remote stream wrappers.

    @pbuyle devised an improvement in #105 which we should adopt:

    function file_directory_is_writable($uri) {
      // By converting the URI to a normal path using drupal_realpath(), we can
      // correctly handle both stream wrappers and normal paths.
      $realpath =  drupal_realpath($uri);
      return is_writable($realpath ? $realpath : $uri) && drupal_is_executable($uri);
    }
    

    This is also going to need tests. The tests for file_is_writable and file_is_executable can be very simple.

    More difficult is the test for the underlying stream wrapper issue. Can we reproduce this? Can we find a uri that returns false for is_writable() but converts OK using drupal_realpath()?

    Version: 8.5.x-dev » 8.6.x-dev

    Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    solotandem’s picture

    Patch removes calls to drupal_realpath($uri) as this is unnecessary and introduces a guaranteed failure for any remote stream wrapper. Even if core does not support a remote wrapper this function will be used by contributed modules and it will NOT solve the problem in all cases.

    Also, I have not found is_writable() to be at issue here, only the lack of an is_executable() check (which the discussion herein seems to agree with as well as inspection of the PHP source code).

    Status: Needs review » Needs work

    The last submitted patch, 133: 944582-133-add-executable-check.patch, failed testing. View results

    solotandem’s picture

    Status: Needs work » Needs review
    FileSize
    15.85 KB

    Last patch inadvertently had a 7.x function call.

    Status: Needs review » Needs work

    The last submitted patch, 135: 944582-135-add-executable-check.patch, failed testing. View results

    solotandem’s picture

    Status: Needs work » Needs review
    FileSize
    15.9 KB

    Attempt to fix test fail.

    Status: Needs review » Needs work

    The last submitted patch, 137: 944582-137-add-executable-check.patch, failed testing. View results

    solotandem’s picture

    Status: Needs work » Needs review
    FileSize
    15.91 KB

    Third time could be the charm.

    Status: Needs review » Needs work

    The last submitted patch, 139: 944582-139-add-executable-check.patch, failed testing. View results

    seanB’s picture

    Status: Needs work » Needs review
    FileSize
    16.08 KB

    Rerolled #130 for 8.5 and 8.6 (works on both). Added 2 extra changes for core/modules/media/media.install and core/modules/migrate/src/Plugin/migrate/process/FileCopy.php. Moved the change from core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php to core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php.

    Status: Needs review » Needs work

    The last submitted patch, 141: 944582-141-reroll-8.5-8.6.patch, failed testing. View results

    seanB’s picture

    So \Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() is called by core/scripts/run-tests.sh while core/includes/file.inc is not included yet. Not sure what the best fix is? I guess we should include file.inc but I'm not sure how core/scripts/run-tests.sh will handle this.

    When I change core/scripts/run-tests.sh I still get an error when running the tests:

    PHP Fatal error:  Uncaught Error: Call to undefined function Drupal\Tests\Listeners\file_directory_is_writable() in /var/www/html/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php:30
    Stack trace:
    #0 /var/www/html/core/tests/Drupal/Tests/Listeners/Legacy/HtmlOutputPrinter.php(21): Drupal\Tests\Listeners\Legacy\HtmlOutputPrinter->setUpHtmlOutput()
    #1 /var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(240): Drupal\Tests\Listeners\Legacy\HtmlOutputPrinter->__construct(NULL, false, 'auto', false, 80)
    #2 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(149): PHPUnit_TextUI_TestRunner->doRun(Object(PHPUnit_Framework_TestSuite), Array)
    #3 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(100): PHPUnit_TextUI_Command->run(Array, true)
    #4 /var/www/html/vendor/phpunit/phpunit/phpunit(52): PHPUnit_TextUI_Command::main()
    #5 {main}  thrown in /var/www/html/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php on line 30
    

    Not sure if anyone has any ideas?

    seanB’s picture

    Status: Needs work » Needs review
    FileSize
    16.12 KB
    1.59 KB

    Reverted the changes in \Drupal\Component\FileSystem\FileSystem and Drupal\Tests\Listeners\HtmlOutputPrinterTrait and added a native is_executable() call to both to fix the tests. Components can't rely on Drupal being available.

    Let's see if this works.

    Status: Needs review » Needs work

    The last submitted patch, 144: 944582-144.patch, failed testing. View results

    jonathanshaw’s picture

    +++ b/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php
    @@ -27,7 +27,7 @@ protected function setUpHtmlOutput() {
             $this->writeWithColor('bg-red, fg-black', "HTML output directory $html_output_directory is not a writable directory.");
    

    "not a writable and executable directory"?

    Pascal-’s picture

    This patch solves an issue with Lando, where you are unable to upload any files using the CMS.
    You can upload one file, after that you're no longer allowed to upload anything.
    If you remove your files folder you're allowed one upload again.

    This patch seems to fix it.
    I'm allowed to upload files again.

    Tested against Drupal 8.4.4

    jofitz’s picture

    Status: Needs work » Needs review
    FileSize
    16.3 KB
    1.18 KB

    Added the improvement proposed in #105.

    is_executable() always returns FALSE for a directory, so I have changed drupal_is_executable() to also check for is_dir() and added a comment to explain this.

    Status: Needs review » Needs work

    The last submitted patch, 148: 944582-148.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    slydevil’s picture

    Version: 8.6.x-dev » 7.x-dev
    slydevil’s picture

    Version: 7.x-dev » 8.6.x-dev

    Reverting, updated the wrong ticket.

    Pascal-’s picture

    Priority: Major » Critical

    Changing this to critical since it's blocking development on Docker / Lando

    I'm not sure if this is for all Docker applications, but when using Lando, it's not possible to upload files using the CMS because of this permission check.

    Would be awesome if we could get a working patch into Core asap!

    anavarre’s picture

    Priority: Critical » Major

    Reverting priority according to https://www.drupal.org/core/issue-priority

    it's not possible to upload files using the CMS because of this permission check.

    I don't have this issue against HEAD. Please provide more details and steps to reproduce.

    Pascal-’s picture

    @anavarre

    Set up a Drupal 8 site with Lando/Lando (I'm running Windows 10)
    https://github.com/lando/lando

    Create a content type with an image field.

    Add content, upload image (works first time)
    Try to add another piece of content with image (image will no longer upload)

    The first time it manages to create the files folder and upload the image.
    The second time it checks the permissions on the files folder and concludes it is not allowed to upload there (I assume)

    Edit: this is on Windows 10

    anavarre’s picture

    Sorry, I can't reproduce with Lando or any other LAMP stack. I'm running Linux so I'd be better if someone else running Windows 10 would test and report back here.

    Pascal-’s picture

    I have 2 colleagues with Windows 10 who have the same issue.
    The issue also persists over multiple projects.

    maacl’s picture

    Working with file permissions for files in a volume mapping in Docker on Windows is very limited. More Information and discussion (and other workarounds) can be found here: https://github.com/wodby/docker4drupal/issues/29#issuecomment-316055081

    anavarre’s picture

    @maacl makes a good point. I'd recommend you try with a non-containerized env (e.g. WAMP) or try on simplytest.me to confirm you're no longer seeing the issue.

    maacl’s picture

    @anavarre actually the patch from this issue should resolve the problem in a containerized env on Docker on Win10 (as explained in https://github.com/wodby/docker4drupal/issues/29#issuecomment-316055081), so this is why @Pascal is asking for commit/progress.

    jonathanshaw’s picture

    Yeah, the issue is real for docker on Windows 10.

    Pascal-’s picture

    @maacl & anavarre
    It works fine on WAMP (without patching) or with this patch on Docker
    This patch does indeed solve the issue on docker, that's why I'd love to see this comitted.

    I'm sure there are a lot of people having the same issue who are unable to find this bug report since it doesn't seem related.
    There's issues about it on Lando and Docker too!

    jonathanshaw’s picture

    It can't be committed yet, it needs tests.

    Pascal-’s picture

    Just ran into the exact same issue on Drupal 7 with Windows 10 + Docker (Lando)
    Patch it #70 applied and fixed it!

    Pascal-’s picture

    Should there be separate D7 & D8 issues for this?
    Could we add something about Docker + Windows 10 to the title so people can actually find this issue?

    cilefen’s picture

    Title: ./sites/default/files directory permission check is incorrect during install AND status report » ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows)
    Issue tags: +Windows, +docker
    BBC’s picture

    Trying on Lando on Windows 10 and ran into the same issue. Hadn't tried uploading yet, but was unable to enable the Media module on a clean install. Can confirm that #144 fixes the issue.

    Version: 8.6.x-dev » 8.7.x-dev

    Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    DrDam’s picture

    #144 is OK for RTBC for me

    idebr’s picture

    Status: Needs work » Needs review
    FileSize
    16.03 KB

    Status: Needs review » Needs work

    The last submitted patch, 169: 944582-169.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Version: 8.7.x-dev » 8.8.x-dev

    Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    das-peter’s picture

    Status: Needs work » Needs review
    FileSize
    18.04 KB

    Re-rolled for latest 8.7x branch.
    Since file_prepare_directory() now is in Drupal/Component/FileSystem/FileSystem.php I've decided to add the new writable / executable checks to that service too. I've kept file_directory_is_writable and drupal_is_executable in place but they now use the service \Drupal::service('file_system'). For now I didn't add a deprecation notice even thought that might be "necessary" to align with file_prepare_directory().

    das-peter’s picture

    FileSize
    18.03 KB

    Hmm, the previous patch had a typo that broke it. Not happy that it seemed to have passed the tests nonetheless :|
    Fixed the typo and corrected the reported coding standard violation too.

    Status: Needs review » Needs work

    The last submitted patch, 173: 944582-173.patch, failed testing. View results

    jonathanshaw’s picture

    +++ b/core/includes/file.inc
    @@ -317,6 +317,46 @@ function file_prepare_directory(&$directory, $options = FileSystemInterface::MOD
    +function file_directory_is_writable($uri) {
    +  return \Drupal::service('file_system')->isWritable($uri);
    +}
    ...
    +function drupal_is_executable($uri) {
    +  return \Drupal::service('file_system')->isExecutable($uri);
    +}
    

    My understanding is that we're moving away from global functions towards OOP; therefore I doubt we should be creating new ones if there's no absolute need to do so.

    Could we just use the new service instead of calling these wrapper functions?

    Pascal-’s picture

    FYI This is no longer an issue with the latest Docker version on Windows.
    Well, the issue is probably still there, but it's no longer affecting uploading images/files

    Not sure if Docker fixed it or if it was fixed in Drupal somewhere.
    I did still have the issue on the older Docker version (with the latest Drupal version), so I'm guessing Docker.

    Version: 8.8.x-dev » 8.9.x-dev

    Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    das-peter’s picture

    Status: Needs work » Needs review
    FileSize
    19.8 KB

    Updated patch to apply to 8.8.x / 8.9.x.

    My understanding is that we're moving away from global functions towards OOP;

    This patch has been around for a long time and I've to assume that there are usages that rely on the procedural functions - hence I've just adjusted the approach to match the current setup which throws deprecated warnings in case the procedural functions are used.
    However, the core usage relies on the extended FileSystemInterface.

    Status: Needs review » Needs work

    The last submitted patch, 178: 944582-178.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Version: 8.9.x-dev » 9.1.x-dev

    Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    thomscode’s picture

    Re-rolling patch for Drupal 8.8 and 8.9.

    Version: 9.1.x-dev » 9.2.x-dev

    Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

    Version: 9.2.x-dev » 9.3.x-dev

    Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Pinolo’s picture

    Pinolo’s picture

    Version: 9.3.x-dev » 9.4.x-dev

    Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    ShaneOnABike’s picture

    Version: 9.4.x-dev » 9.5.x-dev

    Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.5.x-dev » 10.1.x-dev

    Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    rkcreation’s picture

    Hi!

    Patch #185 fixes issues for me (mainly in Linux ACL context, but also weird issues that make image styles not available...)

    is_writable('translations://') return false, but now FileSystem::isWritable('translations://') return true :-)

    Well done! Need this in core!

    Version: 10.1.x-dev » 11.x-dev

    Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    vesnag’s picture

    Thank you IliaNoz for this patch. It works fine.