File API Stream Wrapper Conversion

subscribing.

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.

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.

subscribing. great work!

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

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.

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.

part 1 consolidated here

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

resetting the title

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.

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.

subscribe

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

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

The last submitted patch failed testing.

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

The last submitted patch failed testing.

Wrong indentation.

I do not understand what this comment means.

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

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

Prepend htaccess with a dot (.htaccess).

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

Please use

(and elsewhere)

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

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

Duplicate newline.

Typo in "specified".

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

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.

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.

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.

The last submitted patch failed testing.

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().

The last submitted patch failed testing.

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

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

Attaching.

The last submitted patch failed testing.

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

The last submitted patch failed testing.

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

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

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

The last submitted patch failed testing.

Some more fixes. Love us!

took another pass over the code and comments

The last submitted patch failed testing.

oops - left prefixes in the patch

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

sync with CVS HEAD and fix minor merge conflict.

The last submitted patch failed testing.

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

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

Shouldn't this be ensure?

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

Style guide says to use
'elseif'

Same thing here: 'elseif'

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.

elseif

80 char comments

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

This review is powered by Dreditor.

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

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

@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.

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

Sync with CVS HEAD.

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?

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

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

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

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

@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. :)

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.

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

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?

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?

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...

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?

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.

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.

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?

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.

Sorry. Resetting status.

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.

"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.

"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?

: 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.

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/'?

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.

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.

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.

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

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.

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.

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).

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

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!

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.

Oops.

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

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.

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

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:

Some have shown interest in #445990: Color module needs to be removed from core. 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?

@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.

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).

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

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.

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

@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.

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

+

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

subscribing

This caused: #577398: File API broke SimpleTest temporary file handling

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.

Just noticed this and this looks appropriate for mentioning these patches waiting for review:
#573300: system_retrieve_file() fails in file_unmanaged_copy() (invocation with path instead of URI)
#573292: enable file_unmanaged_delete() to handle stream wrapper URIs
#573288: File.inc function documentation: clarify that several functions _must_ be fed a stream wrapper URI
#563382: when editing image style, link to sample image broken without clean urls (missing file_create_url())

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.

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?

@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.

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

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

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.

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

@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)?

#653068: Update documentation is incomplete