Figure out what to do about new private/public file separation

Well, what about defaulting to "all files public" as in all previous Drupal versions, and to use private files, you first have to enable them by either entering a path or specifying to use sites/default/files/private. So this would be a non-obligatory step (not every site needs private files), therefore we don't have to present an error, but can rather provide a friendly-looking link "If you want to use private, protected file downloads on your site, please proceed to the private files settings page". There, we then have a nice page with lots of space for help text and explanations. Could basically look like
---------------------------------------------------------------------------------
Set up private files

( ) Enter a path to a writeable directory outside of your webroot
[............]
( ) Use sites/default/files/private. WARNING: This may be insecure on systems other than Apache.
[submit]
---------------------------------------------------------------------------------

Of course with better texts, and we could also only offer the second option after we have tested that files stored there are only accessible via system/files (could work similar to clean URL check).

I agree with Frando.

Private files is turned on to meet a specific functionality requirement, so it makes sense to have only Public files configured initially. Letting users know the option is there after install is the best we can do. All the other options end up sacrificing UX somewhere or end up being insecure depending on the web server.

More detail:

#1 -- Agreed, there's just no way to ensure the proper access across all web server software and configurations.

#2 -- I haven't gone through the D7 installation process in a while, I mostly just lurk reading issues, but I'm inclined to agree that adding more options degrades UX. Perhaps it could somehow be made an option for the Expert installation profile. Think of most software installers on Windows. If it just keep hitting next it will install the software's default components in a default location. Determining a default location outside of the web root could be done with some comparison of the site URL and local path (probably), but in most cases PHP won't have access to do anything in the directory above web root. Adding another "go here and chmod this" during installation sounds like a bad idea.

#3 -- I think it's fine to leave public there, but private shouldn't be enabled by the installer. Having an error after installation would be very dissatisfying and a turn off. Again, I'm imagining installing some new software on my desktop only to have it throw missing DLL errors or something upon first run. Not pleasant.

there are some simple tests that have been created for private streams now, which is part of (but not the only reason) we now enable a private:// stream wrapper in addition to the public:// stream by default. so if we remove that, we'll probably need to rework the tests.

Is this why currently in HEAD the installer mentions missing 'private' and 'private/temp' directories?

Private files is turned on to meet a specific functionality requirement, so it makes sense to have only Public files configured initially. Letting users know the option is there after install is the best we can do. All the other options end up sacrificing UX somewhere or end up being insecure depending on the web server.

Sounds like UX and DX both point to the same solution :)

The stream wrappers patch added a temp directory managed by Drupal - this as to avoid problems with using the system temp directory under some configurations.

The installer will currently put this under sites/default/private/temporary - hence you must create sites/default/private for any file uploads to work.

This temp dir is also projected by just a .htaccess file, but generally the names are random using drupal_tempnam(), and unless you turn on private files, all the files here are destined to be public eventually.

Unless a non-core module uses private files by default, you'd have to navigate to the settings page and turn them on, so in a sense we already require you to turn this on before private:// is used for a new install. I guess we could essentially have private:// fail unless the user has specifically set a directory?

The installer shouldn't be prompting for more than one directory, as it currently seems to, so promoting this to bug report.

Bump, this is super critical - its pretty much a hassle to install Drupal now. You have to figure out from the rather cryptic help text what categories to make.

How about having an blank index.php page being created in the private directory? That would disable directory indexes without the need for .htaccess files, but it doesn't solve the problem for people who know the path to the private file.

axyjo - not really a solution

First pass at a patch - note that it bumps the PHP requirement to insure we have function sys_get_temp_dir().

If we don't want to bump that, then we'll have to add a fallback or something.

Also, by default this puts the private files under the public one - surely not where you'd really want them, but makes install simpler. I won't be surprised if this makes the testbot sad also.

I think it's better to be able to install, than it is to have a great default path for private file directory - if you really care you'll stick that outside the webroot.

5.2.1 - are there any distributions which still ship with this? Any hosts still running it? I can't imagine there are but you never know.

As of > 1 year ago, there were still some people using 5.2.0: http://www.nexen.net/images/stories/phpversion/200810/mineure5.en.png

Don't seem to be more recent updates at that site.

PHP version change looks fine to me.

I don't see why the installer has to create a private directory at all. All our profiles default to public. Why not default the private variable to ../private or something outside of web root?

@moshe - I don't feel strongly either way - but seems nicer to at least have a functioning path by default.

@webchick - we could do that, but seems like we might have to add some kind of error handling?

perhaps we could add a switch to enable the private stream, and leave it off by default? turning it on would be from the private files setting page, which would also require a valid folder on verify.

@19 can't have the textfield for private file path with an invalid value set, since the first time you load the form you don't see any errors (haven't been submitted therefore not validation yet)
- leaving the page at that point (without submitting anything) will be a WTF
- attempting to "reset to default" the page will be another WTF

that's what I think aaron is aiming at #18, #20

This patch creates a new setting to allow private files. If it's not set, then we don't have private files.

It is set with a checkbox on the file system admin page at admin/config/media/file-system. Checking it and hitting submit then displays the usual textfield for the directory. That also rebuilds the menu, so we can register the new private files menu path.

I put the default path at ../private/files, and everything works fine from my end.

I forgot to add that the variable to register the private stream wrapper is initially set to FALSE, of course.

@aaron - that misses the changes for the temporary files directory.

Also - do we really need a setting?

I think we could make this change much simpler with an empty string as the default directory. I'm also wary of the menu rebuild and toggling 'system/files' in an out of existence.

Also, what does PHP do if we reference a non-registered stream wrapper?

Here's what I mean - think I got most of it, though possible tests fail due to missing stream. Tested install and configure.

We should probably rethink the files form and its validation (rather than use the after build trick?)

Also, quick test - trying to use a non-registered wrapper gives this notice:

Notice: is_dir(): Unable to find the wrapper "private" - did you forget to enable it when you configured PHP? in main() (line 21 of /Users/Shared/www/drupal-7/index.php).

doh, my test hack was still in index.php

Looks like we'd need to fix up a lot of tests.

Can we put #12 in as a stepping stone to fixing this? At the least it allows you to install with only have one files dir present.

Can we perhaps use our lovely conditional AJAX-Y thing? When you check, it exposes the field to enter a path? :) We have it in there, so lets use it.

you can find the code in menu's I think, if you add a menu by node/add - you get the same trick

@Bojhan - I have no idea what you are talking about in this context. You mean w.r.t. aaron's patch to add an additional setting?

He refers to the #states feature that we added to FAPI which lets you easily make form elements be dependant on each other. #557272: FAPI: JavaScript States system

@moshe - ok, but I don't think that's relevant if we use the approach in my patch.

#30 wouldn't work anyway, because we'd get form validation errors before we even began because the directory probably won't exist at first.

note I opened this related issue for a more general problem with the stream wrapper API: #685074: (Tests needed) Some stream wrappers need to be hidden or read-only

We can possibly merge that with this fix if we come to any quick consensus there.

For the #27 patch - a quick and dirty trick to make the most tests pass would be to set a private files path like 'file/private' at the start of the tests so that they have a writeable directory to work with.

I reviewed this patch and I'd support the approach taken here. My only comment was whether we still need the variable 'file_temporary_path'? It seems like we might be able to always use sys_get_temp_dir(), but that is a minor comment at this stage.

 // By default no private files directory is configured.
+    variable_get('file_private_path', FALSE),

and by extension

   foreach ($directories as $directory) {
+    if (!$directory) {
+      continue;
+    }

might be removed?

I think it's useful to be able to set some other temporary file path - e.g. for the testbot or for cases where you are on shared hosting and not allowed to access /tmp. The latter case was the original motivation for putting the default temp file locaiton under the docroot.

Back to needs review, it seems Dries his comment was responded to by pwolanin. Can anyone see why its failing so many test, somewhat hesitant to retest - since our bot was broken so many times.

@Bojan - we need to either fix the tests or define the private files path.

/me trows up a coin, fix the tests it is! Please, lets get these critical issues fixed.

5.2.1 - are there any distributions which still ship with this?

Debian Etch shipped 5.2.0. It was superseded by Debian Lenny about a year ago but is still supported with security updates.

In D6, file_directory_temp() did something like sys_get_temp_dir() in PHP 5.2.1. I think it's a pity to drop support for 5.2.0 when there is an easy workaround.

-  else {
+  elseif(is_dir($directory)) {

Missing space before (.

I'd rather narrow the number of supported PHP versions if possible.

Also, even security patchs for Debian Etch are now over, so it's unsupported and past it's EOL date:
Security Support for Debian GNU/Linux 4.0 to be terminated on February 15th

Came here from issue #722510: Private directories should default to a directory outside of the webroot that I created today. Now set to duplicate.

I think we should either do:

1). If the installer can create the directories, we can set it up like 'WEBROOT/../drupalprivate/[site name]/files' and 'WEBROOT/../drupalprivate/[site name]/temp'. This keeps the /sites/[sitename]/files setup we have for public directories.

2). If the installer can't do it automatically (meaning not on most web hosts), I think we should leave it like it is in D6 and just send a message that it can be done with a link to the file system config page. This informs the user of the feature but keeps the installer uncluttered. Most users will still use the public method and the private file system is not required to get drupal to run.

So can I get screens, and is there a RTBC'able patch given the directions given by webchick

#27: files-dirs-551658-27.patch queued for re-testing.

Update: Looks like #685074: (Tests needed) Some stream wrappers need to be hidden or read-only conflicts with this patch.

The patch conflicts or the approach?

patch conflicts... it seems pretty minor.

Patch no longer applies.

Rerolled around the conflicts. Also made one addition to the docs explaining why we don't create the private files directory by default.

Looks good to me. No errors during install, no option to set the default file system to private until a path was set, no problems creating the path I specified, and no problems using it once it was set.

@webchick - the sweet merciful crap is a Mac OS thing - on a "normal" system like linux it would be something like "/tmp"

Maybe makes sense avoid showing a single radio - you'll see two radios if you configure a private files path.

the "/Applications/MAMP/tmp/php" is a MAMP thing not an OS X thing.

look at the image toolkits form for a pattern of how to do the radio buttons.

wait ignore my temp directory comment.

My Mac gives me like /var/folders/Yy/YyjK01R92Rabfk+BYqOrF++++TM/-Tmp-/ for sys_get_temp_dir() regardless of whether I use MAMP or the system PHP. In contrast, linux gives me: /tmp . So even though /tmp works on the Mac, for some reason it's not what the system wants you to use.

http://api.drupal.org/api/function/system_image_toolkit_settings/7 is a nearly criminal abuse of the API (variable_set() in a form builder).

the bit about the access callback for the private files path is wrong. That would prevent any other module (not file) from implementing an alternative system of private downloads.

I rolled this with a removal of the singe radio, but in IRC webchick didn't like the screenshot so much, so this patch just has other fixes but omits any UI refactoring.

we weren't using sys_get_temp_dir() http://php.net/manual/en/function.sys-get-temp-dir.php before now, as that function wasn't introduced in php until 5.2.1. previously, unless you were in windows, it was usually set to /tmp manually with http://api.drupal.org/api/function/file_directory_temp/6 . sys_get_temp_dir() should be safer to use, even if your system does something weird: afaik, mac creates a symlink from /tmp to the same folder.

only problem i see is that it silently breaks if you create a folder named 0 and set it as the private folder. seems a really odd and edge case though, not enough to stop this from happening. :D

Just a small followup, INSTALL.txt should state that PHP 5.2.1 is the minimum version now as well.

good catch!

Just in case this is useful information to anyone, (apparently) as a result of this patch going in, I got this error in the "verify requirements" step of the install when I did a fresh D7 install the other day.

The directory /var/folders/3I/3IfJxJtFFrWrCXQPxLjD8++++TI/-Tmp- is not writable.

From what I gathered with the help of chx and some others on IRC, it seems it likely has something to do with me having 2 copies of Apache on my computer (one in MAMP and one that came with my macbook). I never did get it figured out yet, and just chmod'ed the perms on that folder to get my install to complete so I could get on with things.

Not sure whether I should file an issue for this or if it's just me and a bad setup.

Clients always confuse the different between:

Public local files served by the webserver / Drupal

they thought all served by SERVER

@webchick - I was hoping that this would allow success in installing on any platform withot hard-coding a list of possible dirs - also is more compact.

However, I'd also be open to rolling that part back.

I don't fully understand all the technical arguments but that this is critical should be crystal clear. Currently new users are faced with way too many steps; added by the private map. One typically has to go through 4/5 steps before one gets to having the correct directions setup.

This patch is to solve this issue, that prevents people from installing Drupal 7. I would definitely mark this very very critical.

I disagree rolling back the temp dir stuff. php offers an .ini setting where sites configure this stuff. is drupal supposed to just ignore that and say 'we know better. use /tmp until you do a drupal specific configuration for something that php already offers'?

FWIW, I use mac localhost and am not having any trouble.

I have no strong feelings on this. My .02 is that using sys_get_temp_dir() is not a major change, and it is a bug fix.

@webchick Heh, I think you confused me too. I was thinking you where proposing a full rollback. If not all fine :)

I think this patch might be causing a problem with testing.

On my setup I am getting exceptions if the host does not have private:// configured:
file_exists(): Unable to find the wrapper "private" - did you forget to enable it when you configured PHP?

Try testing cron run.

I don't know how many people are, or are not, using Debian Etch. I use it on my dev server and it would be a pain for me, and perhaps others, to upgrade to php 5.2.1+. I'm not saying it's right, just that it is a fact, and this could degrade impressions of d7.

If the 5.2.1 requirement is going to stay, then we need an actual UI to explain this in the installer. Currently after selecting language I get

Fatal error: Call to undefined function sys_get_temp_dir()

See also:

#807622: Fatal error calling sys_get_temp_dir() in installer - pHP 5.2.0 and earlier

The use of sys_get_temp_dir assumes this temporary directory is writable. Is this a correct assumption? On IIS the system temp dir is not writable for the php process, instead, one is supposed to use per-application pool temporary directories, distinct from the system temp dir. I can imagine this is a problem with Unices as well.

The change needs some additional documentation IMO (eg How to change the reported path with lowlevel access rights.)

The use of sys_get_temp_dir by default is a mistake. Web hosting providers can change PHP's upload_tmp_dir setting on a per user/website basis and if set Drupal should be using it. Security conscious users might also want to set their own upload_tmp_dir for Drupal to use.

sys_get_temp_dir ignores the upload_tmp_dir setting and returns the default temp directory instead.

So it really should go something like this I think: First check if upload_tmp_dir is set. If it is, use it. If it isn't, use sys_get_temp_dir.

Makes sense. At least that is what the late http://api.drupal.org/api/function/file_directory_temp/6 used to do.

Hmm... not really. Looking at http://php.net/manual/en/function.sys-get-temp-dir.php, this function should return PHP's temp directory.

But looking at the comments on the php.net page, it seems either unreliable or easy to misuse.

Ok, looks like we should pull that Drupal 6 code forward to Drupal 7.

See also #807622: Fatal error calling sys_get_temp_dir() in installer - pHP 5.2.0 and earlier.

Alpha 5 will not install for me!!

I have the SAME setup and issue as [#68], and CANNOT get past the install screen. For some reason my Mac's default TMP directory is not writable by the MAMP web server. I understand that there are workarounds such as chmoding the rather cryptic TMP directory, or setting Drupal's "upload_tmp_dir" variable, but workarounds are not solutions.

If "sys_get_temp_dir()" is being used to get this directory, and we cannot install on MAMP because of it, then I really think this is not acceptable code for a Drupal release. Even if we assume MAMP is only used on local dev servers, and not for production, this bug would still cause irritation (at least) for a large number of developers.

IMHO we're back to critical (cause Drupal won't install now, where before it would), and 7.0-alpha5
M.M.

Resetting version, all bugs are against dev.

BTW ...
the Drupal variable to set as a workaround is "file_temporary_path", not "upload_tmp_dir" as I stated above. I set this variable in my settings.php file, and all is well.
$conf['file_temporary_path'] = '/tmp';

M.M.

With the latest dev version of D7, I ran into this temp directory problem as well. I couldn't proceed with the install because '/tmp' could not be written to (because of open basedir restrictions). On shared Linux hosting, letting PHP applications use the '/tmp' directory is riddled with security issues. Because of that, on our systems each web account has it's own temp directory. In apache, we have the following PHP variables set:

"upload_tmp_dir" "/var/www/web254/phptmp/"
"session.save_path" "/var/www/web254/phptmp/"

From my perspective, Drupal should use the value of 'upload_tmp_dir' if it's set, then fall back to the value of sys_get_temp_dir(), which hopefully returns a fairly sane result.

In any case, I heeded manimejia's advice and added $conf['file_temporary_path'] = '/var/www/web254/phptmp' to my settings.php file in order to allow the installation to proceed.

-Ian

Here's a more or less straight forward port of the Drupal 6 code.

Turns out there was a totally pointless test case too.

I now get the MAMP php temp dir on a fresh install:

just testing this and i still has the fail described in comment 68 http://drupal.org/node/551658#comment-2919160

screenshot http://skitch.com/arianek/d8ees/requirements-problem-drupal

fwiw yoroy just tested on the same env. and it installed fine, so it seems to be something specific to my setup. booo.

discussing with arianek in IRC - I have the same version of MAMP and it works ok, plus it works for yoroy, so I suspect there is something broken in her setup.

Strange, why did is_writable() let that directory be selected in the loop and then it was reported as non-writable. Is that a temp name or something.

What does print ini_get('open_basedir'); say?

will this file api change be backportet to d6?

No, new APIs and API changes are never backported. But this issue here is only about default settings and file positions.

Patch looks good, I don't have a mac to test on but other people do, RTBC.

Shouldn't the minimum php version be 5.2.1, as sys_get_temp_dir() does not exist in earlier versions, but it is used in includes/file.inc and modules/system/system.install?

in file.inc, it is used in the definition file_directory_temp(), so it certainly shouldn't be replaced by a call to file_directory_temp(), since that would cause an infinite loop. We could add a check to see if sys_get_temp_path() exists before using it to get another potential temp path.

In system.install, it is used in system_requirements. file_directory_temp() can't be used here as it requires a database connection to save the variable file_directory_temp to the database, which is not yet available during the requirements checking phase of the install.

I've included a patch which addresses these issues, but these are rather hacky.

Why do we even need to check for this during the install? I don't think Drupal 6 does.

Adding the function_exists() call, we can keep the min PHP version back down at 5.2.0, though I'd honestly rather leave it at 5.2.1 to narrow a little the range of buggy PHP versions we pretend to support.

IMO sys_get_temp_dir is not a strong reason enough to require 5.2.1, it is just a really minor version
and function_exists with a fallback behavior wouldn't be that bad

@pwolanin: The installer tries to create the directories, and gives an error message if this fails, telling the user to create the directory automatically. This prevents the user having finished the installation and immediately receiving an error message about the temp path.

I agree with arhak on the version. It would be a pity requiring php 5.2.1, but only using the functionality it adds during install.

Why "immediately receiving an error message about the temp path"?

There are always things to configure after installation, when you want to use the feature. In D6 this was one of them.

I'm not sure why. The private path is not checked (unless it is in settings.php), but the temporary path is. pwolanin changed the private path checking behavior in the patch in #551658-62: Figure out what to do about new private/public file separation, so maybe he knows the rationale behind this?

A guess is that file_directory_temp() might otherwise return a temporary directory which can't be used? There are some remarks about this in the first few comments of this issue.

The temporary stream wrapper is used in includes/updater.inc (update system?). That may be an important difference between the temporary and the private path, which isn't used in core by default.

There is a usability gamble here.
- If everything goes smoothly it is a plus.
- If it fails, the user is given no option except fixing the suggested directory, if possible at all.
- If the user was given an option (a) that would delay a successful installation and (b) would rush the user to take action outside any context that would help to make an informed decision.

I kind of feel as though we should remove the temp directory check from the install requirements for the sake of simplicity. We could leave it in the runtime requirements, for example, so you'd see an error if it's not working, but much Drupal functionality does not need a temp dir so it should not block installation, and almost everyone is going to get a working temp dir with no effort (sorry Ariane).

i be cursed! ...will get someone to help me fix it eventually (if it's just me, no point in messing with things!)

#109 makes sense, and also brings the chance to provide better guidance/help/choices without making the installation process embroiled (as cog.rusty points out @#108)
(that was the reason to leave private directory to post-installation setup)

Here is a new patch based on the comments above. Removed from the requirements at install time, but still in the requirements at runtime. This also removes the need to check for install phase in file_directory_temp.

Not sure about the formatting of the code comment. Otherwise looks good.

Changed the comment a bit

Another comment change, after discussion with pwolanin on irc

We still need to include file.inc in the patch.

And get rid of a trailing space in the comment, this should be the last one :)

+  // We don't check

Just "Do not check" is code style I think.

+    // This function exists in PHP 5 >= 5.2.1, but Drupal
+    // requires PHP 5 >= 5.2.0, so we check for it

Missing a full stop.

I think it's fine otherwise.

Another reroll. My eye fell on some other small code/comment style issues in file_directory_temp, so I'm slightly abusing this patch to fix these as well :)

CNR for the bot, looks ready to go when it comes back green.

rtbc because others said so :)

Committed to CVS HEAD. Thanks.

it lives! i had no idea whether that change would fix my crazy issue, but i just updated to HEAD and my install completed. happyday. :-)

hm, these changes created quite some major weirdness, #866756: file_directory_path('private') returns FALSE by default

Effectively, I'm not really sure what the benefit here is.

Before: Drupal installed with a private files directory by default. Modules are able to use that. Users can configure and reconfigure.

After: No private files directory exists by default. Modules should be using what-exactly to store their private files now? Fall back to 'public'? What's the idea behind that? :)

I can understand that, when using advanced/other stream wrappers, it's a module's responsibility to check whether file_directory_path('awesome') actually returns something valid to store in, but I absolutely expect and actually have to expect that file_directory_path('private') works at any time.

I'm tempted to re-open this issue and request a complete rollback, but we can discuss first.

The minimum PHP version was set to 5.2.4 in #938614: Downgrade required PHP version to 5.2.4. As far as I can tell, sys_get_temp_dir() is defined in all versions of PHP >= 5.2.1. It would seem that a function_exists() check is wasteful if that situation, but problems were reported in this issue. webchick's comment from http://drupal.org/node/938614#comment-3620386 indicates that a possible Mac OSX issue exists. Now that the minimum PHP version is 5.2.4, has the OSX issue resurfaced?

jrchamp: I think the OSX issue is because of directory returned by sys_get_temp_dir() is not writable in some scenario. It is addressed by file_directory_temp() as it checks if it is writable. So function_exists() is useless, and the comment about 5.2.0 obsolete.