This issue is a split from #227232: Support PHP stream wrappers. This patch will refractor the File API and other core modules to fully support stream wrappers.

The attached patch is only for reference. Issues caused by the image cache commits over the weekend have not been resolved yet.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

subscribing.

jmstacey’s picture

Woot! We've shed nearly 20k and are finally back under the 200k barrier :-)

file_directory_temp() is back and file_directory_path() transparently adds new functionality so that we don't have to change all function calls.

I'll be splitting this patch one more time so that we'll have two File API patches (parts 1 and 2) later today, so stay tuned.

jmstacey’s picture

Title: File API Stream Wrapper Support » File API Stream Wrapper Support (Part 2)
FileSize
175.14 KB

The easily separable function additions that were not tied to the file object and DB changes have been moved to #519392: File API Stream Wrapper Support (Part 1). This is now designated as part 2 of the patch sequence.

Dependencies: #227232: Support PHP stream wrappers and #519392: File API Stream Wrapper Support (Part 1).

To test this patch
The testing bot will not be able to test this patch until dependencies are committed. In the mean time, you can test this patch locally by applying in the following sequence: #227232: Support PHP stream wrappers, #519392: File API Stream Wrapper Support (Part 1), this patch.

aaron’s picture

subscribing. great work!

jmstacey’s picture

FileSize
172.41 KB

Some stuff was moved upstream to one of the other patches. There have been several conflicted merges that I have not tested.

We'll need to keep an eye on #391330: File Field for Core

jmstacey’s picture

FileSize
146.02 KB

I've split this patch once again: #527002: File API Stream Wrapper Support (Part 3). The new patch contains refreshed function definitions for the changes in this patch. Note however, that the function definitions are still correct in this patch and have been updated as needed, but only minimally. The bulk of styling, rewording, and example changes have been moved to the new patch. This should make both patches easier to review by removing 26k of documentation changes from this patch.

The new patch is derived from the fileapi-3_styling branch on GitHub. These styling changes are still different than the more generic ones found in the styling_corrections branch.

Anonymous’s picture

creating a summary here for Dries based on discussions at the file sprint. here's a hit list of functions:

function file_create_url($uri)

rename to file_create_external_url(), but otherwise leave as is. the use case here is for unmanaged files.

function file_create_path($uri = NULL, $stream = TRUE)

make this function like file_create_external_url(), where you pass in a $uri and get back the real path to the file on the file system. again for unmanaged files, use case here is gd library which doesn't handle the stream wrapper $uri correctly.

function file_check_directory(&$directory, $mode = 0, $form_item = NULL)

this is a monster that should go away. we will create:

file_create_htaccess($dir, $private = FALSE) - create a htaccess file in the given directory

drupal_mkdir - we wrap mkdir() because php doesn't pass the drupal default mode into the stream wrappers, instead it passes the php default, which is 777.

the form coupling just needs to die, and be moved into the system settings form code. the directory exists/ directory is writable can be handled by is_dir()/is_writable()

function file_check_path(&$path)

this just dies, if you want to know if something is a directory, use is_dir()

function file_check_location($source, $directory = '')

this is currently only called by file_check_path(). file_check_path()'s functionality will be handled by the local stream wrapper DrupalLocalStreamWrapper.

function file_directory_strip($path)

this is not called anywhere in core and can be deleted. we may want to add a function later that accepts a full file system path and hands back a stream wrapper target.

pwolanin’s picture

Title: File API Stream Wrapper Support (Part 2) » File API Stream Wrapper Conversion

part 1 consolidated here

drewish’s picture

Title: File API Stream Wrapper Conversion » File API Stream Wrapper Support (Part 2)

moved this over to http://wiki.github.com/jmstacey/drupal/to-do-list so we could all edit it.

drewish’s picture

Title: File API Stream Wrapper Support (Part 2) » File API Stream Wrapper Conversion

resetting the title

Anonymous’s picture

woo! good to see the first patch go in.

i'm in ohio at a training until friday, but i can work on this all day on friday while lurking at the back of the room at DrupalDelphia drupal camp.

quicksketch’s picture

Subscribing. I don't think this will effect #391330: File Field for Core much at all, since we'll only be using existing API functions like file_save() and file_create_url(), which hopefully will all be updated by this patch. Great work so far, very exciting work.

moshe weitzman’s picture

subscribe

pwolanin’s picture

Here's the current patch (needs work) from the git repo. This represents much of the work at the sprint in Philly.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
234.44 KB

After work by jmstacey, others, and me - now at least some tests pass... sacrificiing to testbot to see where it fails.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
235.31 KB

try again with a few more changes by jmstacey after discussion with boombatower reveals no clear reason for this total fail on testbot.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

+++ includes/file.inc
@@ -220,6 +200,8 @@ function file_uri_target($uri) {
+ * @return
+ *	 Returns a string containing the normalized URI.

Wrong indentation.

+++ includes/file.inc
@@ -299,93 +283,81 @@ function file_stream_wrapper_get_instance_by_scheme($scheme) {
+ * Compatibility: normal paths and stream wrappers.
+ * @see http://drupal.org/node/515192

I do not understand what this comment means.

+++ includes/file.inc
@@ -299,93 +283,81 @@ function file_stream_wrapper_get_instance_by_scheme($scheme) {
+ *   and the same string is returned. If a Valid stream wrapper could not be found

This line slightly exceeds 80 chars and "Valid" should be lower-case here.

+++ includes/file.inc
@@ -299,93 +283,81 @@ function file_stream_wrapper_get_instance_by_scheme($scheme) {
+    // If this is not a properly formatted stream return the URI with the base url prepended.
...
+    // Check for http so that we don't have to implement getExternalUrl() for the http wrapper.
@@ -587,7 +518,9 @@ function file_save($file) {
  * @param $destination
- *   A string containing the destination that $source should be copied to. This
+ *   A string containing the destination that $source should be copied to. This should
+ *   be a stream wrapper URI. If this value is omitted, Drupal's public files directory
+ *   will be used [public://].
@@ -670,38 +604,53 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+  // Determine whether or not we can perform this operation based on overwrite rules.
@@ -897,33 +865,44 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) {
+ *   Returns the filepath consisting of $directory and a unique filename
+ *   based off of $basename.
  */
 function file_create_filename($basename, $directory) {
@@ -958,8 +937,8 @@ function file_create_filename($basename, $directory) {
+ * @return
+ *   Returns TRUE for success, FALSE in the event of an error, or an array if the file
...
@@ -982,25 +961,28 @@ function file_delete($file, $force = FALSE) {
@@ -1098,8 +1087,10 @@ function file_space_used($uid = NULL, $status = FILE_STATUS_PERMANENT) {
  * @param $destination
- *   A string containing the directory $source should be copied to. If this is
- *   not provided or is not writable, the temporary directory will be used.
+ *   A string containing the location $source should be copied to within the temp directory.
+ *   $destination can be a directory or directory and filename within the temp directory.
+ *   If not provided, the temporary directory will be used, and the $replace flag will
+ *   will be honored.
@@ -1107,8 +1098,8 @@ function file_space_used($uid = NULL, $status = FILE_STATUS_PERMANENT) {
 function file_save_upload($source, $validators = array(), $destination = FALSE, $replace = FILE_EXISTS_RENAME) {
+++ modules/image/image.module
@@ -547,7 +548,7 @@ function image_style_flush($style) {
- * image at image/generate/[style_name]/[path] the image is generated if it does
+ * image at image/generate/[style_name]/[scheme]/[path] the image is generated if it does
...
@@ -565,7 +566,7 @@ function image_style_url($style_name, $path) {
+++ modules/system/image.gd.inc
@@ -254,6 +254,11 @@ function image_gd_load(stdClass $image) {
 function image_gd_save(stdClass $image, $destination) {
+  // Convert URI to a normal path because PHP apparently has some gaps in stream wrapper support.
+++ modules/system/system.install
@@ -176,49 +176,55 @@ function system_requirements($phase) {
+      // The files directory requirement check is done only during install and runtime.
+++ modules/user/user.module
@@ -404,7 +404,8 @@ function user_save($account, $edit = array(), $category = 'account') {
+        // TODO: Lookup a private/public setting. Or user_picture_path should include private:// or public://.
+++ modules/user/user.test
@@ -517,13 +517,17 @@ class UserPictureTestCase extends DrupalWebTestCase {
+      TODO: Add capability to store photos publicly or privately, but tests will be the same.

These comments do not wrap at 80 chars. (tried to provide context where possible)

+++ includes/file.inc
@@ -395,106 +367,65 @@ function file_check_directory(&$directory, $mode = 0, $form_item = NULL) {
+ * Creates an htaccess file in the given directory.
...
+    // Private htaccess file.
...
+    // Public htaccess file.

Prepend htaccess with a dot (.htaccess).

+++ includes/file.inc
@@ -395,106 +367,65 @@ function file_check_directory(&$directory, $mode = 0, $form_item = NULL) {
- * @param $conditions
- *   An array of conditions to match against the {files} table. These
+ * @param array $conditions
+ *   An array of conditions to match against the {file} table. These
@@ -537,12 +468,12 @@ function file_load($fid) {
- * @param $file
+ * @param object $file

Please remove the variable type (array/object) before parameter name.

+++ includes/file.inc
@@ -670,38 +604,53 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+    // TODO: Replace drupal_set_message() calls with exceptions instead.

Please use

// @todo Your comment here.

(and elsewhere)

+++ includes/file.inc
@@ -670,38 +604,53 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+  

(and elsewhere) Trailing white-space (blank lines should be blank).

+++ includes/file.inc
@@ -714,9 +663,21 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+ * Given a relative path, construct a uri into Drupal's default files location.
+ */
+function file_build_uri($path) {

"uri" should be upper-case in the PHPDoc summary.

+++ includes/file.inc
@@ -714,9 +663,21 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+
+

Duplicate newline.

+++ includes/file.inc
@@ -726,8 +687,8 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+ *   and FILE_EXISTS_ERROR is speciefied.
  */
 function file_destination($destination, $replace) {

Typo in "specified".

+++ includes/file.inc
@@ -751,7 +712,7 @@ function file_destination($destination, $replace) {
+ * Moves a file to a new location and update the file's database entry.
...
@@ -761,6 +722,9 @@ function file_destination($destination, $replace) {

"Moves" + "updates" (third-person form).

+++ includes/file.inc
@@ -822,9 +787,12 @@ function file_move($source, $destination = NULL, $replace = FILE_EXISTS_RENAME)
- * Move a file to a new location without calling any hooks or making any
+ * Moves a file to a new location without calling any hooks or making any
  * changes to the database.
@@ -851,9 +819,9 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
- * Munge the filename as needed for security purposes.
+ * Munges the filename as needed for security purposes.
  *
- * For instance the file name "exploit.php.pps" would become "exploit.php_.pps".
+ * For example, the file name "exploit.php.pps" would become "exploit.php_.pps".
@@ -862,7 +830,7 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
  * @return
- *   $filename The potentially modified $filename.
+ *   Returns the potentially modified file name.
@@ -897,33 +865,44 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) {
- * Undo the effect of upload_munge_filename().
+ * Reverses the effect of file_munge_filename().
...
- *   An unmunged filename string.
+ *   Returns the unmunged file name.

PHPDoc summary should be on one line.

That said, this patch could be *much* decreased in size if those adjustments were left out (deferred to a novice patch)... Additionally, some of these changes don't make sense ("@return Returns...").

Because of that, I stopped reviewing here, because it is very hard to review so many unrelated changes.

20 days to code freeze. Better review yourself.

chx’s picture

Also I see $pic_path = ' that's too many spaces... this I saw in the sibling issue too which now is duplicate to this. jmstacey, pwolanin please carry on we need this before the freeze.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
238.69 KB

this doesn't address sun's comments yet - just trying to see f our code as of last night will be applied by the test-bot aftre merging in new core changes.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
238.58 KB

thanks to chx for the suggestion to try the CLI runner - there was a fatal error in the CLI test runner due to incorrect conversion of realpath() to drupal_realpath().

Status: Needs review » Needs work

The last submitted patch failed testing.

jmstacey’s picture

FileSize
239.39 KB

We've done a bit of hacking, let's see what's left to fix.

jmstacey’s picture

Status: Needs work » Needs review

We've done a bit of hacking, let's see what's left to fix.

jmstacey’s picture

FileSize
239.39 KB

Attaching.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
242.57 KB

Image stuff is really broken still, but maybe fixed the uplaod module fails?

Status: Needs review » Needs work

The last submitted patch failed testing.

jmstacey’s picture

Status: Needs work » Needs review
FileSize
299.69 KB

New roll from the work this morning. Let's see where we stand.

jmstacey’s picture

FileSize
242.84 KB

Resubmission--the last diff is to an old version of HEAD and will fail.

pwolanin’s picture

The last patch also include the fix in this patch: http://drupal.org/node/547412

Status: Needs review » Needs work

The last submitted patch failed testing.

jmstacey’s picture

Status: Needs work » Needs review
FileSize
243.57 KB

Some more fixes. Love us!

pwolanin’s picture

took another pass over the code and comments

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
229.67 KB

oops - left prefixes in the patch

jmstacey’s picture

FileSize
229.56 KB

This cleans up some left over cruft from the file_create_path() removal and a few other tweaks and is updated to current HEAD.

pwolanin’s picture

sync with CVS HEAD and fix minor merge conflict.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
229.64 KB

for some reasona a scheme was "pictures://" instead of "public://"

axyjo’s picture

Status: Needs review » Needs work
+++ includes/file.inc
@@ -299,56 +285,42 @@ function file_stream_wrapper_get_instance_by_scheme($scheme) {
+  // TODO: Implement CDN integration hook stuff in this function.
+  // see http://drupal.org/node/499156

Just reiterating sun's point - use @todo. There's other places as well.

+++ includes/file.inc
@@ -357,126 +329,90 @@ function file_create_path($destination = NULL) {
+function file_insure_htaccess() {

Shouldn't this be ensure?

+++ includes/file.inc
@@ -587,7 +523,9 @@ function file_save($file) {
+ *   A string containing the destination that $source should be copied to. This should
+ *   be a stream wrapper URI. If this value is omitted, Drupal's public files directory

Core usually limits comments to 80 characters to a line. I could be wrong though.

+++ includes/file.inc
@@ -623,7 +561,7 @@ function file_copy($source, $destination = NULL, $replace = FILE_EXISTS_RENAME)
+    else if ($replace == FILE_EXISTS_RENAME && is_file($destination)) {

Style guide says to use
'elseif'

+++ includes/file.inc
@@ -802,7 +764,7 @@ function file_move($source, $destination = NULL, $replace = FILE_EXISTS_RENAME)
+    else if ($replace == FILE_EXISTS_RENAME && is_file($destination)) {

Same thing here: 'elseif'

+++ includes/file.inc
@@ -1442,7 +1418,7 @@ function file_validate_image_resolution($file, $maximum_dimensions = 0, $minimum
+ *   A string containing the destination URI. If no value is provided
  *   then a randomly name will be generated and the file saved in Drupal's
  *   files directory.

I don't know if this is valid or not, but we could probably move the word 'then' to the line above and get the word 'files' a line above as well.

+++ includes/file.inc
@@ -1481,7 +1457,7 @@ function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAM
+    else if ($replace == FILE_EXISTS_RENAME && is_file($destination)) {

elseif

+++ modules/image/image.module
@@ -547,7 +548,7 @@ function image_style_flush($style) {
- * image at image/generate/[style_name]/[path] the image is generated if it does
+ * image at image/generate/[style_name]/[scheme]/[path] the image is generated if it does

80 char comments

This is my first patch review. How did I do? Was I too harsh?

This review is powered by Dreditor.

pwolanin’s picture

@axyjo - harsh? Hardly - did you review the actual code changes too?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
230.22 KB

renamed function to file_ensure_htaccess() and clean up comments/style as suggested.

axyjo’s picture

@pwolanin - Nope, I haven't tested out the patch on HEAD. I'll try to do it. I wasn't sure how critical I needed to be.

jmstacey’s picture

@axyjo, the more testing the better :-). I'll be doing some click testing tomorrow as well.

jmstacey’s picture

FileSize
230.23 KB

Sync with CVS HEAD.

Dries’s picture

Given the size of this patch, I'm keen to commit it as soon as possible and to follow-up with patches as necessary.

It's a bit hard to track but has sun's feedback been incorporated?

pwolanin’s picture

@Dries- yes, I think I addressed all of sun's comments in the patch in #36 if not before.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

Great, than I think we should commit this, and follow-up with smaller patches. Any objections? :)

jmstacey’s picture

Synced with CVS HEAD again. Some minor remaining cleanups from sun's review and INSTALL.txt has been updated.

pwolanin’s picture

@Dries - ok, with #52, we should be good I think.

webchick’s picture

@Dries: No objections here, although I've not looked too closely at this or the other patch yet, so I'll let you do the honours. :)

webchick’s picture

Well. I was going to just commit this tonight, because I don't want to be held up committing other things, which meant I needed to sneak a peek. It seems like this direction has already been signed off on from Dries, and that's fine. I did figure I should stick my initial thoughts here, though.

+++ INSTALL.txt
@@ -127,10 +127,13 @@ INSTALLATION
+     mkdir sites/default/private
+     chmod o+w sites/default/private

Um? NO! You don't put your private files directory in a web-accessible place!

+++ includes/common.inc
@@ -2601,7 +2601,7 @@ function drupal_get_css($css = NULL) {
-          $rendered_css[] = '<link type="text/css" rel="stylesheet" media="' . $item['media'] . '" href="' . base_path() . $item['data'] . $query_string . '" />';
+          $rendered_css[] = '<link type="text/css" rel="stylesheet" media="' . $item['media'] . '" href="' . file_external_url($item['data']) . $query_string . '" />';

Er. But it's *not* an external URL. It's part of my own site's files. This is terribly confusing. Is there a way we can make a distinction between internal, non-managed files and external, off-on-some-other site files?

+++ includes/file.inc
@@ -24,7 +24,7 @@ require_once DRUPAL_ROOT . '/includes/stream_wrappers.inc';
- * - filepath - Path of the file relative to Drupal root.
+ * - uri - URI of the file.

In general, hunks like this worry me. Everyone on the planet can make sense of files and directories and think of accessing a file in those terms. But thinking of files as URLs (sorry, UR*I*s, which is even less natural :P) with made-up protocols is totally unnatural to the way that people work with files in any other system. I lament that our sweet, rockin' better-in-every-way file API that we've had around Jan/Feb is now going to be replaced with this new system which, although I can see it creates all kinds of interesting opportunities for developers doing advanced media handling, is going to slap all the rest of the Drupal developers squarely in the face with a nice, big WTF. On the plus side, the D7 version of Pro Drupal Development will sell record numbers, I'm sure. ;)

Sorry, just had to get that off my chest. Carry on. :P

I'm on crack. Are you, too?

drewish’s picture

webchick, we protect the private files with a .htaccess file.

the external means it's the URL that would accessible externally, like you could give the URL to someone and they could access it, as opposed to an internal URL that only makes sense on the site.

the whole point of the renaming is that flickr://photos/3812947245 isn't a file path. but it is a URI...

Dries’s picture

Mmm, webchick has a good point especially because .htaccess is not available on all systems. Can private files (optionally) be stored in a private directory too?

Are public files stored in chmod o+w sites/default/public for consistency?

drewish’s picture

The only problem with trying to tell people where to put a private files directory it that it's really hard to write docs for that. We don't know the layout of their filesystem, if they've got open basedir restrictions, where they can create directories, etc. The upside though is that you can now easily move the directory and just update the setting and everything keeps working. So yeah it's a problem but this makes it much much easier to fix.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I'm still skeptical about the proposed solution for private files -- relying on .htaccess is a bad idea. If I'm using Drupal for an extranet, I wouldn't want my private company files to be protected by just a .htaccess file. That scares the hell out of me. We've added various new security features to Drupal 7, but this looks like a regression in terms of file security. I agree with webchick that we need to not store private files in a public directory. Fortunately, it sounds like it would be easy to fix. Let's get it fixed first.

drewish’s picture

Dries, where would you suggest we tell people to put their private files directory? what's a good default that will work on the majority of hosts?

if that's not easy how about a requirement that uses cURL to fetch a private file and reports an error if it succeeds?

webchick’s picture

Status: Needs work » Reviewed & tested by the community

drewish: I'd say that most hosts have the concept of a web-accessible vs. web inaccessible directory. So you could use a "for example" where the website was in:

/home/example/public_html

and they should put their private files outside of the directory, for example:

/home/example/private_files

Then mention that if a non-web-accessible directory is unavailable, that it will be protected via .htaccess on Apache servers.

I don't think it's up to this documentation to explain this in much more detail than that.. all of this is easily googleable.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry. Resetting status.

sun.core’s picture

Status: Needs work » Needs review
pwolanin’s picture

In Drupal 6 if you install Drupal and then turn on private file downloads, your files driectory will always have been installed in a web-accessible location and won't even be pretected by a .htaccess file or anything. So, I don't think it's a regression, and documentation around private files is currently quite lacking. I'd agree the new docs here could be better, but do you actually want to prompt the users for the private files location during the install? Otherwise we can only put it somewhere under the Drupal root, and hence web-accessible.

@webchick - do you have a suggestion for a better name for file_external_url()? As drewish says, this function generates the address that you can put in a web browser to get the file. The current function is called file_create_url(), but this seems even more misleading, which is why we wanted to change it.

webchick’s picture

"In Drupal 6 if you install Drupal and then turn on private file downloads, your files driectory will always have been installed in a web-accessible location."

Yes, but you'll change the location of the files directory accordingly when you adjust the setting to Private, since both are on the same screen. The help text even tells me to do that: "If the download method is set to private, this directory should not be accessible over the web."

"but do you actually want to promot the users for the private files location during the install?"

I should probably try this patch on my DreamHost account, but I don't see how we avoid prompting them for this during install anyway with the current patch? I guess unless you set the entire sites/default folder writable, but you would not want to do this since modules and themes can also go in there. So IMO this needs to happen either way, and we might as well impart best practices, or at the very least not recommend worst practices.

Keeping files intended to be private outside of web root is seriously something that has been drilled into my skull for 15 years as a web developer, and if I encountered a software package telling me to put them in a public-facing directory, that would be the last time I used said software package. :P

"do you have a suggestion for a better name for file_external_url()? As drewish says, this function gnerates the address that you can put in a web browser to get the file."

Honestly, I would've just left it at "file_create_url()". It currently makes sense -- you're creating a URL to the file's location. I suppose it gets more confusing now that we reference files by what most people would call "URLs", but elsewhere (like drupal_add_css/js) we use "external" to mean "not here - somewhere else on the internet." Not mixing these two up is especially important since these a lot of these line changes were in themes/preprocess functions, and themers are going to be exposed to the CSS/JS definitions of 'external' and have preconceived notions about what that means.

This issue unfortunately doesn't have any of the back-story that I could find as to why this name switch was done so I don't know the specific rationale, but I would say just switch it back to file_create_url(), and if someone can come up with a really awesome and non-confusing alternative name for that function before code freeze, we can switch it.

Anyway, it wasn't my intention to derail this. I still agree with Dries that committing it sooner than later is best.

webchick’s picture

"but do you actually want to promot the users for the private files location during the install?"

Duh. The LOCATION. Not the directory creation. Yay, 3 hours of sleep! ;)

Hmmm... I agree that this needs some thought around UX as far as how to present this. Seems our only alternatives would be to either prompt them for a file location during installation or else put it in sites/default and then show a warning in the status messages panel to change this.

Else, is there any way we could assume "one directory above this directory"/private_files as a sensible default path?

aaron’s picture

: Else, is there any way we could assume "one directory above this directory"/private_files as a sensible default path?

I think it's even more unlikely that such a directory would be writable by Drupal by default.

I would hope that most developers with a need to create a private file system would know enough to be able to create the folder first, and we could have the google-search hints to help them through the rest of it.

I like your suggestion of a warning in the status messages panel as a good compromise for now.

jmstacey’s picture

I completely agree about the private files being outside a web-accessible location. Unfortunately it has the side effect of scattering files on the system, so instead of only backing up sites/, users now have to remember a second directory (or more for multi-site). That's something we'll have to get used to I guess.

I'm leaning towards resolving the issue after installation via the status panel. Users will probably already have an alert telling them that settings.php is not secure, and we can suggest where to place the directory. If we go that route what do we use for the variable_get() default? Something like '~/site.example.com/private/'?

pwolanin’s picture

The default download method is still private, so on a new install to change it to private they'd still have to go to this admin screen where they could also change the path of the private directory.

Depending on the server setup, folders under the Drupal root might be readable by PHP, but not by the webserver - or the entire Drupal install might be outside the webroot (isn't that the point of the D7 DRUPAL_ROOT constant?). We could try to test whether the private files area is readable by issuing a http request, though this seems a little iffy.

sun’s picture

Sorry, but now we are in total scope creep. This patch is about introducing stream wrappers - not about improving the UX, documentation, README.txt, or installation instructions to properly set up private files with Drupal. Especially, because the old instructions did the same. We can move that discussion into a separate issue, and if you think it's absolutely required to improve those, then we mark that new issue as a critical task.

sun’s picture

Status: Needs review » Reviewed & tested by the community
+  $source = drupal_realpath($source);
...
+    $destination = file_build_uri(basename($source));
...
 function file_create_filename($basename, $directory) {
...
+  drupal_chmod($file->uri);
...
+  $temp_name = drupal_tempnam('temporary://', 'file');
...
+function drupal_dirname($uri) {
...
+function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
...
+function drupal_tempnam($directory, $prefix) {
...
+    ' src="' . file_external_url($element['#src']) . '" ' .

Next to some remaining coding standards issues, I additionally think that we should re-think some more function names, because I find above excerpt quite confusing.

However, reviewing, discussing, and deciding that with a monster patch like this takes too much time, IMHO. I would like to suggest to create a separate, critical "File API clean-up" issue to tackle those issues separately. That way, everyone can have a look at the new code, grok it, and come up with better suggestions for particular stuff. Searching + replacing function names is also much easier once this patch landed (contrary to re-roll + especially review this monster).

This review is powered by Dreditor.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
227.25 KB

This patch syncs CVS head (and resolves a minor conflict) and reverts back to file_create_url() as the function name.

webchick’s picture

pwolanin: Cool. When testing bot comes back green, I'll go ahead and commit this. I agree that sorting out this whole UX stuff around where to put private files is turning into its own green, three-headed monster of an issue that we should likely explore separately.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

One last question. Who gets commit credit on this? Is everyone who helped out at the media sprint represented in this issue?

This is what I was going to type:

#517814 by jmstacey, justinrandell, pwolanin: Added File API Stream Wrapper Conversion.

drewish’s picture

I'd suggest that we need to credit the folks who worked on #227232: Support PHP stream wrappers it looks like c960657 is the only one missing from there, and dopry (still quite a bit of code from the media.module).

pwolanin’s picture

also Jody Lynn was at the sprint and did quite a lot of work on this part. so the list probably should include:

jmstacey, justinrandell, pwolanin, drewish, Jody Lynn, aaron, dopry, c960657

Dries’s picture

I'll let webchick commit this then, but let's create a follow-up issue to deal with the private files issue. Thanks for the great work!

webchick’s picture

Issue tags: +Needs documentation

Ok. Committed to HEAD! Great work everyone. I realize in my previous response I sounded very negative but I *really* do appreciate that this makes private/public file handling actually rock. :D Drupal 7 is going to be awesome!!!

The follow-up about private files is at #551658: Figure out what to do about new private/public file separation. I don't think additional follow-ups are necessary; we can just upload any other minor style clean-up patches here.

Marking "code needs work" for upgrade documentation.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Oops.

webchick’s picture

For anyone documenting this patch, reply #7 has a pretty good summary.

quicksketch’s picture

This patch broke file_save_upload() in subdirectories: #552066: file_save_upload() does not work in sub-directories (without trailing slash). Documentation is desperately necessary, as this borked file.module a lot more than I was expecting due to all the function renaming and the complete change of $file->filename to $file->uri.

c960657’s picture

This caused a regression in the image module when the default download method is private - see issue #443200 comment #14.

roychri’s picture

This patch seem to have broke the color module.

The color module is using file_unmanaged_copy() which require a uri with a scheme. One of the file is in themes/garland/images so the public/private/temporary schemes are not good.

When I use the color module (patched with #497948: Color module is broken), then I get these errors like this:

The specified file themes/garland/images/menu-collapsed.gif could not be copied, because the destination sites/default/files/color/garland-2ba69380/menu-collapsed.gif is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper.

Some have shown interest in #445990: [META] Refactor color module. I do not mind fixing the color module if 1) we are going to keep it in core 2) I know which scheme to use.

Maybe I should not use file_unmanaged_copy() but rather something else?

aaron’s picture

@roychri

I hadn't thought of that case, and I don't recall it coming up in discussion.

We should definitely continue to use file_unmanaged_copy(), as you don't want a full file object for that use case. However, maybe we should create a drupal:// stream for files in the webroot directory tree?

In any case, that definitely needs its own issue; this issue is too large already.

aaron’s picture

That's also a good case for a read-only stream wrapper instance, which would definitely be needed for a drupal:// (in addition to being useful for many third party content providers).

aaron’s picture

@c960657 -- does the aforementioned patch fix the problem? or is there something needed to change with stream wrappers to deal with it?

jmstacey’s picture

roychri, only the destination parameter of file_unmanaged_copy() must be a stream. The source can still be a normal path such as 'themes/garland/images/menu-collapsed.gif'.

Off the top of my head: file_unmanaged_copy() may have been creating the destination directory structure previously and this no longer happens automatically. One solution is to explicitly call file_prepare_directory() with the FILE_CREATE_DIRECTORY option to create the structure before calling file_unmanaged_copy().

Disclaimer: I haven't looked at the issue in depth so I might be missing some details.

aaron’s picture

i stand corrected... thanks for jumping on that, jmstacey :D

roychri’s picture

@jmstacey - Thank you for the information. This is exacly what I needed.
I actually do not need to callfile_repare_directory() since the color module was already doing it.
I will submit a new patch to #497948: Color module is broken once I test more and cleanup.

Thank you.

c960657’s picture

@aaron - yes, the latest patch for #443200: User pictures does not work with private files fixes the regression.

hass’s picture

+

pwolanin’s picture

Another possible bug to look into as a follow-up: #555364: Set a testing temp directory as well as public/private

hapydoyzer’s picture

subscribing

boombatower’s picture

jhodgdon’s picture

The function header doc for the functions that changed in this patch wasn't fixed up either. eMPee584 reported this #566798: Documentation problem with file_scan_directory: filepath => uri, which I've closed as a duplicate. The function doc currently says:

key' The key to be used for the returned array of files. Possible values are 'filepath', for the path starting with $dir, 'filename', for the basename of the file, and 'name' for the name of the file without an extension. Defaults to 'filepath'.

wrong: It's uri now.

So this needs to be fixed also, in addition to the broken functionality.

Dave Reid’s picture

This sorely needs some documentation in the 6.x to 7.x upgrade handbook (http://drupal.org/update/modules/6/7). It took me 10 minutes of searching through the cvs commits of includes/file.inc to figure out why/how file_directory_temp() was replaced in D7.

jhodgdon’s picture

Category: feature » bug

At this point, I think the "feature" part of this has been taken care of, and what is remaining is to fix the bugs with how it was done, and get it documented -- correct?

jmstacey’s picture

@Dave Reid,
Oops, that got pushed to the side when school started :-( I'll try to squeeze in some time this weekend to give that a kickstart.

clojel’s picture

upload_file_download returns -1 when a private file is not associated with node

  if ($file && user_access('view uploaded files') && ($node = node_load($file->nid)) && node_access('view', $node)) {
    return array(
      'Content-Type' => $file->filemime,
      'Content-Length' => $file->filesize,
    );
  }
  else {
    return -1;
  }

As a result, when attempt to download private files that are not associated with node, we will always get Access Denied (see file_download)

    // Let other modules provide headers and controls access to the file.
    $headers = module_invoke_all('file_download', $uri);
    if (in_array(-1, $headers)) {
      return drupal_access_denied();
    }

i wonder if the else statement could be removed from upload_file_download. It would make way for future contrib module(s) that uses file module independently of node (eg. node_import from previous drupal).

Thank you for your attention.
Please advise.

pwolanin’s picture

@jmstacey - any update? the documentation is really the last step of getting the patch in.

jmstacey’s picture

@pwolanin, Too much bread and not enough butter :-P. I did get started on the File API section some time back (http://drupal.org/node/555118). There's a semi-comprehensive list (ignoring argument changes) here: http://wiki.github.com/jmstacey/drupal/to-do-list
I just haven't had the time. Should they go under the latest unstable-# section or the one it actually happened in (was that unstable 7, 8)?

catch’s picture

Priority: Critical » Normal
c960657’s picture

giuseppegiache’s picture

Status: Needs work » Needs review

fileapi_stream_wrappers.patch queued for re-testing.

dman’s picture

(partial) documentation update and coder_upgrade issue #741970: file_check_directory() renamed to file_prepare_directory()

naught101’s picture

marcoka’s picture

are there any plans on a backport for d6?

jhodgdon’s picture

RE #109 - The stream wrapper API isn't present in Drupal 6, and will not be added. We don't make major changes like this in production versions of Drupal. Sorry! Drupal 7 will be out when:
http://drupal.org/project/drupal

joachim’s picture

Priority: Normal » Critical

Critical problem with documentation, as pointed out here: #613918: file_create_path and file_check_directory vanished with no upgrade notes.

The upgrade notes http://drupal.org/update/modules/6/7 are full of examples citing 'file_create_path' and it doesn't exist any more.

chx’s picture

Project: Drupal core » Documentation
Version: 7.x-dev »
Component: file system » Correction/Clarification
jhodgdon’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: Correction/Clarification » file system

I think we are keeping the issues related to the 6/7 upgrade docs in the Drupal queue rather than Documentation project queue, because they need to be fixed by devs.

catch’s picture

Priority: Critical » Normal
Status: Needs review » Needs work
andypost’s picture

Any reason that simpletest_test_stream_wrappers() hook in simpletest.module named differently - suppose it's name should be simpletest_stream_wrappers()

andypost’s picture

jhodgdon’s picture

OK...

This issue is still open because the upgrade modules page is very broken. Anyone care to fix it?

chx’s picture

rfay’s picture

OK, the easy way to start this is for somebody to write a blog post about stream wrappers. Or an example module. This is a nontrivial change, and we need to figure out what the change was and communicate it to people.

Does anybody have a blog post up their sleeves?

"Stream wrappers in Drupal 7"?
"Changes in file handling from Drupal 6 to Drupal 7"?

Can anybody recommend an excellent instructive example in core?

rfay’s picture

jhodgdon’s picture

See also #112 above - this issue is staying open because all over the upgrade page, it's using a function that NO LONGER EXISTS due to this issue's commits.

That's independent of needing documentation of this issue's changes and the stream wrapper API, as described in #121 (and the new issue).

jhodgdon’s picture

Also, we normally keep Drupal Core issues open until the upgrade docs have been added. That other issue is in the Documentation project, which means that probably no developer who could write the doc will ever see it, unless people here subscribe. So let's continue to track the upgrade docs here, and leave that other issue to writing a section in the Drupal APIs part of the Handbook for stream wrappers. Which is definitely needed.

rfay’s picture

Reviews of #874364: Create a file example demonstrating key file APIs and the streamwrapper concept will be appreciated. It's a demonstration of several file APIs and a near-complete demonstration stream wrapper implementation.

I'm working to consolidate what I learned and the D6->D7 changes so they can end up as docs.

eMPee584’s picture

wow gee this stuff is powerful

rfay’s picture

Status: Needs work » Fixed

I've attempted to provide docs:

Your review and improvement on those is welcome. There is more to be done, and there are inaccuracies and errors of course.

I'm going to mark this fixed - unmark it if you like.

webchick’s picture

OMG. Awesome soufflé, drizzled in awesome sauce, with a slice of awesome cream pie for dessert. THANK YOU, RANDY!!!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

anrikun’s picture

Status: Closed (fixed) » Needs work

Bug at #112 is still present.

The upgrade notes http://drupal.org/update/modules/6/7 are full of examples citing 'file_create_path' and it doesn't exist any more.

jhodgdon’s picture

fixing tag for upgrade docs

jhodgdon’s picture

Status: Needs work » Fixed

RE #129 - thanks for noticing this!

According to the update page
http://drupal.org/node/877214
anywhere that uses file_create_path() in d6 should be changed to file_prepare_directory(FILE_CREATE_DIRECTORY) in D7. So I went through the 6/7 update guide and fixed the examples.

I found two places, and fixed both of them.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

pillarsdotnet’s picture

Status: Closed (fixed) » Needs work

Dunno the proper place to post this, but I just discovered that the D6 'pathinfo' field was renamed to 'uri' in D7 and I can't find the change mentioned anywhere in Converting 6.x modules to 7.x.

Also filed #1332960: The mimedetect_mime() function assumes a $file->filepath property, but it does not exist.

jhodgdon’s picture

Status: Needs work » Closed (fixed)

Please file a separate issue, and when you do so, please mention where the 'pathinfo' field is used (what d6/7 functions).

pillarsdotnet’s picture

It's a database field, not a function parameter. And I've already posted a separate issue against the function that was using it wrongly. My point is, nobody bothered to document the change made here in the update documentation. According to standard Drupal policy, that means this issue should be re-opened as "needs work" until the docs reflect the change made here.

But since we have a plethora of standards, both written and unwritten, I've opened a new issue as you request.

See #1334398: The rename of {files}.pathinfo to {files}.uri is not mentioned in the update documentation.

andypost’s picture

Status: Closed (fixed) » Needs work

as ImageCache Profiles maintaibner I have a lot of questions at #370776: Default picture not showing
So we really need a change notification from native english speaker

jhodgdon’s picture

OK. Please add a change notification detailing this change then. We aren't adding to the 6/7 updates page any more.

pillarsdotnet’s picture

Status: Needs work » Needs review

Change notice added; please review.

jhodgdon’s picture

Status: Needs review » Fixed

Looks OK to me. I'm not sure that all the issues referenced are really relevant to someone looking to understand the change, but maybe...

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: -Needs change record
Chadwick Wood’s picture

Issue summary: View changes

This is related to #2244513: Move the unmanaged file APIs to the file_system service (file.inc). The commit related to this issue here added the @todo: "Replace drupal_set_message() calls with exceptions instead", and this related issue is trying to address that. However it's unclear if making file_unmanaged_move() (and possibly related functions) throw exceptions rather than returning FALSE is clearly the best thing to do, or how that should be achieved consistently.