Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#72 | 517814-fileapi-conversion-71.patch | 227.25 KB | pwolanin |
#52 | 517814-fileapi-conversion-47.patch | 231.04 KB | jmstacey |
#48 | stream_wrapper-46.patch | 230.23 KB | jmstacey |
#45 | 517814-fileapi-conversion-45.patch | 230.22 KB | pwolanin |
#42 | 517814-fileapi-conversion-42.patch | 229.64 KB | pwolanin |
Comments
Comment #1
drewish CreditAttribution: drewish commentedsubscribing.
Comment #2
jmstacey CreditAttribution: jmstacey commentedWoot! 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.
Comment #3
jmstacey CreditAttribution: jmstacey commentedThe 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.
Comment #4
aaron CreditAttribution: aaron commentedsubscribing. great work!
Comment #5
jmstacey CreditAttribution: jmstacey commentedSome 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
Comment #6
jmstacey CreditAttribution: jmstacey commentedI'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.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedcreating 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 directorydrupal_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.
Comment #8
pwolanin CreditAttribution: pwolanin commentedpart 1 consolidated here
Comment #9
drewish CreditAttribution: drewish commentedmoved this over to http://wiki.github.com/jmstacey/drupal/to-do-list so we could all edit it.
Comment #10
drewish CreditAttribution: drewish commentedresetting the title
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedwoo! 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.
Comment #12
quicksketchSubscribing. 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.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe
Comment #14
pwolanin CreditAttribution: pwolanin commentedHere's the current patch (needs work) from the git repo. This represents much of the work at the sprint in Philly.
Comment #15
pwolanin CreditAttribution: pwolanin commentedAfter work by jmstacey, others, and me - now at least some tests pass... sacrificiing to testbot to see where it fails.
Comment #17
pwolanin CreditAttribution: pwolanin commentedtry again with a few more changes by jmstacey after discussion with boombatower reveals no clear reason for this total fail on testbot.
Comment #19
sunWrong 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.
Comment #20
chx CreditAttribution: chx commentedAlso 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.Comment #21
pwolanin CreditAttribution: pwolanin commentedthis 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.
Comment #23
pwolanin CreditAttribution: pwolanin commentedthanks 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().
Comment #25
jmstacey CreditAttribution: jmstacey commentedWe've done a bit of hacking, let's see what's left to fix.
Comment #26
jmstacey CreditAttribution: jmstacey commentedWe've done a bit of hacking, let's see what's left to fix.
Comment #27
jmstacey CreditAttribution: jmstacey commentedAttaching.
Comment #29
pwolanin CreditAttribution: pwolanin commentedImage stuff is really broken still, but maybe fixed the uplaod module fails?
Comment #31
jmstacey CreditAttribution: jmstacey commentedNew roll from the work this morning. Let's see where we stand.
Comment #32
jmstacey CreditAttribution: jmstacey commentedResubmission--the last diff is to an old version of HEAD and will fail.
Comment #33
pwolanin CreditAttribution: pwolanin commentedThe last patch also include the fix in this patch: http://drupal.org/node/547412
Comment #35
jmstacey CreditAttribution: jmstacey commentedSome more fixes. Love us!
Comment #36
pwolanin CreditAttribution: pwolanin commentedtook another pass over the code and comments
Comment #38
pwolanin CreditAttribution: pwolanin commentedoops - left prefixes in the patch
Comment #39
jmstacey CreditAttribution: jmstacey commentedThis cleans up some left over cruft from the file_create_path() removal and a few other tweaks and is updated to current HEAD.
Comment #40
pwolanin CreditAttribution: pwolanin commentedsync with CVS HEAD and fix minor merge conflict.
Comment #42
pwolanin CreditAttribution: pwolanin commentedfor some reasona a scheme was "pictures://" instead of "public://"
Comment #43
axyjo CreditAttribution: axyjo commentedJust 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.
Comment #44
pwolanin CreditAttribution: pwolanin commented@axyjo - harsh? Hardly - did you review the actual code changes too?
Comment #45
pwolanin CreditAttribution: pwolanin commentedrenamed function to file_ensure_htaccess() and clean up comments/style as suggested.
Comment #46
axyjo CreditAttribution: axyjo commented@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.
Comment #47
jmstacey CreditAttribution: jmstacey commented@axyjo, the more testing the better :-). I'll be doing some click testing tomorrow as well.
Comment #48
jmstacey CreditAttribution: jmstacey commentedSync with CVS HEAD.
Comment #49
Dries CreditAttribution: Dries commentedGiven 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?
Comment #50
pwolanin CreditAttribution: pwolanin commented@Dries- yes, I think I addressed all of sun's comments in the patch in #36 if not before.
Comment #51
Dries CreditAttribution: Dries commentedGreat, than I think we should commit this, and follow-up with smaller patches. Any objections? :)
Comment #52
jmstacey CreditAttribution: jmstacey commentedSynced with CVS HEAD again. Some minor remaining cleanups from sun's review and INSTALL.txt has been updated.
Comment #53
pwolanin CreditAttribution: pwolanin commented@Dries - ok, with #52, we should be good I think.
Comment #54
webchick@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. :)
Comment #55
webchickWell. 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?
Comment #56
drewish CreditAttribution: drewish commentedwebchick, 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...
Comment #57
Dries CreditAttribution: Dries commentedMmm, 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?
Comment #58
drewish CreditAttribution: drewish commentedThe 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.
Comment #59
Dries CreditAttribution: Dries commentedI'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.
Comment #60
drewish CreditAttribution: drewish commentedDries, 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?
Comment #61
webchickdrewish: 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.
Comment #62
webchickSorry. Resetting status.
Comment #63
sun.core CreditAttribution: sun.core commentedComment #64
pwolanin CreditAttribution: pwolanin commentedIn 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.
Comment #65
webchick"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.
Comment #66
webchick"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?
Comment #67
aaron CreditAttribution: aaron commented: 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.
Comment #68
jmstacey CreditAttribution: jmstacey commentedI 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/'?
Comment #69
pwolanin CreditAttribution: pwolanin commentedThe 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.
Comment #70
sunSorry, 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.
Comment #71
sunNext 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.
Comment #72
pwolanin CreditAttribution: pwolanin commentedThis patch syncs CVS head (and resolves a minor conflict) and reverts back to file_create_url() as the function name.
Comment #73
webchickpwolanin: 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.
Comment #74
pwolanin CreditAttribution: pwolanin commentedComment #75
webchickOne 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.
Comment #76
drewish CreditAttribution: drewish commentedI'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).
Comment #77
pwolanin CreditAttribution: pwolanin commentedalso 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
Comment #78
Dries CreditAttribution: Dries commentedI'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!
Comment #79
webchickOk. 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.
Comment #80
webchickOops.
Comment #81
webchickFor anyone documenting this patch, reply #7 has a pretty good summary.
Comment #82
quicksketchThis 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.
Comment #83
c960657 CreditAttribution: c960657 commentedThis caused a regression in the image module when the default download method is private - see issue #443200 comment #14.
Comment #84
roychri CreditAttribution: roychri commentedThis 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: [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?
Comment #85
aaron CreditAttribution: aaron commented@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.
Comment #86
aaron CreditAttribution: aaron commentedThat'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).
Comment #87
aaron CreditAttribution: aaron commented@c960657 -- does the aforementioned patch fix the problem? or is there something needed to change with stream wrappers to deal with it?
Comment #88
jmstacey CreditAttribution: jmstacey commentedroychri, 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.
Comment #89
aaron CreditAttribution: aaron commentedi stand corrected... thanks for jumping on that, jmstacey :D
Comment #90
roychri CreditAttribution: roychri commented@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.
Comment #91
c960657 CreditAttribution: c960657 commented@aaron - yes, the latest patch for #443200: User pictures does not work with private files fixes the regression.
Comment #92
hass CreditAttribution: hass commented+
Comment #93
pwolanin CreditAttribution: pwolanin commentedAnother possible bug to look into as a follow-up: #555364: Set a testing temp directory as well as public/private
Comment #94
hapydoyzer CreditAttribution: hapydoyzer commentedsubscribing
Comment #95
boombatower CreditAttribution: boombatower commentedThis caused: #577398: Ensure test coverage of simpletest temporary file handling
Comment #96
jhodgdonThe 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:
wrong: It's uri now.
So this needs to be fixed also, in addition to the broken functionality.
Comment #97
eMPee584 CreditAttribution: eMPee584 commentedJust 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 (missing file_create_url())
Comment #98
Dave ReidThis 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.
Comment #99
jhodgdonAt 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?
Comment #100
jmstacey CreditAttribution: jmstacey commented@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.
Comment #101
clojel CreditAttribution: clojel commentedupload_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.
Comment #102
pwolanin CreditAttribution: pwolanin commented@jmstacey - any update? the documentation is really the last step of getting the patch in.
Comment #103
jmstacey CreditAttribution: jmstacey commented@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)?
Comment #104
catch#653068: Update documentation is incomplete
Comment #105
c960657 CreditAttribution: c960657 commentedFollow-up: #696150: image_gd_save() does not support remote stream wrappers
Comment #106
giuseppegiache CreditAttribution: giuseppegiache commentedfileapi_stream_wrappers.patch queued for re-testing.
Comment #107
dman CreditAttribution: dman commented(partial) documentation update and coder_upgrade issue #741970: file_check_directory() renamed to file_prepare_directory()
Comment #108
naught101 CreditAttribution: naught101 commentedMarked #613918: file_create_path and file_check_directory vanished with no upgrade notes as a duplicate of this.
Comment #109
marcoka CreditAttribution: marcoka commentedare there any plans on a backport for d6?
Comment #110
jhodgdonRE #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
Comment #112
joachim CreditAttribution: joachim commentedCritical 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.
Comment #113
chx CreditAttribution: chx commentedComment #114
jhodgdonI 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.
Comment #115
catch#653068: Update documentation is incomplete
Comment #116
andypostAny reason that simpletest_test_stream_wrappers() hook in simpletest.module named differently - suppose it's name should be simpletest_stream_wrappers()
Comment #117
andypostFollow up issue #867988: Remove simpletest_test_stream_wrappers()
Comment #118
jhodgdonOK...
This issue is still open because the upgrade modules page is very broken. Anyone care to fix it?
Comment #119
chx CreditAttribution: chx commentedNope. Not until #858528: file_uri_target() purpose is unclear and / or #701358: The file API presumes a hiearchical file storage is clarified and / or fixed.
Comment #120
rfayOK, 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?
Comment #121
rfayOpened #872622: Stream Wrappers really, really need documentation and examples
Comment #122
jhodgdonSee 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).
Comment #123
jhodgdonAlso, 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.
Comment #124
rfayReviews 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.
Comment #125
eMPee584 CreditAttribution: eMPee584 commentedwow gee this stuff is powerful
Comment #126
rfayI'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.
Comment #127
webchickOMG. Awesome soufflé, drizzled in awesome sauce, with a slice of awesome cream pie for dessert. THANK YOU, RANDY!!!
Comment #129
anrikun CreditAttribution: anrikun commentedBug at #112 is still present.
Comment #130
jhodgdonfixing tag for upgrade docs
Comment #131
jhodgdonRE #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.
Comment #133
pillarsdotnet CreditAttribution: pillarsdotnet commentedDunno 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.
Comment #134
jhodgdonPlease file a separate issue, and when you do so, please mention where the 'pathinfo' field is used (what d6/7 functions).
Comment #135
pillarsdotnet CreditAttribution: pillarsdotnet commentedIt'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.
Comment #136
andypostas 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
Comment #137
jhodgdonOK. Please add a change notification detailing this change then. We aren't adding to the 6/7 updates page any more.
Comment #138
pillarsdotnet CreditAttribution: pillarsdotnet commentedChange notice added; please review.
Comment #139
jhodgdonLooks OK to me. I'm not sure that all the issues referenced are really relevant to someone looking to understand the change, but maybe...
Comment #141
xjmComment #142
Chadwick Wood CreditAttribution: Chadwick Wood commentedThis 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.