Remove file_directory_path()

+1 from me. I think we should go with this as soon as possible.

This is, of course, a major API breakage for existing contrib.

looks really good; thanks for taking the initiative to help clean up the api!

to be clear, for contrib using it before, they would most likely want to use $wrapper->getDirectoryPath().

all seems good to me. applied the patch and did some basic testing, and nothing seems broken. let's get this thing going!

Looks like a valuable clean-up to me. While we need to remove it, it is questionable as whether we need to remove it from D7. Any reason this can't wait to D8?

This needs to be done in D7 because (almost) every use of it went around the normal "public://" approach, leading to things like deleting things that shouldn't be deleted, etc. It's unfortunate that we discovered this so late, but the clear consensus is that it needs to be removed anyway.

We changed the API a year ago, but unfortunately didn't remove this then, and all usages of this function assume the D6 way of doing things, which just doesn't fly.

The full discussion and reasoning are in #874326: Invalid scheme fails to halt processing of a stream request, with strategy statement in #874326-36: Invalid scheme fails to halt processing of a stream request.

OK, let's bite the bullet then. Committed to CVS HEAD.

Another (less verbose) replacement for contrib is to build the stream-wrapped URI and then use drupal_realpath().

I am using this to pass a filename into imagepng(), which apparently doesn't support stream wrappers. See _color_render_images() as an example.

This needs documentation at http://drupal.org/update/modules/6/7.

Looking at pifr it does all kinds of local file operations in which the functions will not accept stream wrappers, such as chdir(). I would appreciate a replacement for file_directory_path() which is not verbose and silly like (best of Damien's examples) variable_get('file_public_path', conf_path() . '/files').

Having to repeat the default value and what not over a static function is not preferred.

a) needs documentation, b) would be good to clarify what is a good simple replacement.

If drupal_realpath('public://') works then that would be by far the best replacement. Still kinda silly considering 99.9999% of contrib wants the public file directory. Is there no way we can have a special function for that?

file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath() is a cumbersome replacement for file_directory_path().

That's my point....lets make a file_directory_path() a wrapper function for that...waaaay to long.

function file_directory_path() {
  return file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
}

or

function file_directory_path($scheme = 'public') {
  return file_stream_wrapper_get_instance_by_scheme($scheme)->getDirectoryPath();
}

Plus interestingly enough the patch doesn't use your final recommendation.

@mfb: in your case, you want to first check if the file wrapper is local

If you call drupal_realpath('public://mymodule/images') that's guaranteed to be local right? That has been my assumption after looking at core code like color.module...

Maybe best practice is to use temporary:// for operations that don't support stream wrappers and then move the file to the default scheme? For now I am just forcing public:// as e.g. color.module and locale.module do.

@Damien: you are missing the point...if core needs to use various versions of this code that's all well and good...I just want a simple way to get to files directory, nothing more.

In API change notification the alternate is declared as "public://".
AFAIK, it is not accepted by functions such as drupal_add_js/css.
I agree boombatower and think it would be nice to have file_directory_path(SCHEME) back, may be having a different name and documented that it returns the user-defined path.

The "files" directory = public directory...If I choose to assume it is local as my code requires it to be then it won't make a difference which function I use as it will blow up if the public directory is not configured to be local. I don't see what we gain from this...other then confusing people and making them ask the same question I am, "Why did they make this so darn complicated in Drupal 7?"

I mean for all the modules that don't need local files directory they get an easy way to reference the public directory (which everyone uses) and for the few modules that need it to be local they will need to check it on install regardless so forcing people to use some complicated call just because we can't assume the public files directory is local any more really doesn't help the situation in any way.

If we really want to make this slick (not sure how many modules needs this) we could add a flag in the .info file and check automatically in core just like any other dependency (or php version).

For most cases we should probably suggest that people put things in temporary:// directory if they need to use local functions that don't accept stream wrappers and then move the files to public:// or whatever after they are complete. Even then they still want an easy way to get temporary:// path. In my case that is probably what I should do...use temporary path and then either move or symbolic link the "checkout" directory in the public:// files directory, but still having a quick function to get temporary files directory.

So my code would look something like the following.

$temp = ...() . '/mydir';
chdir($temp);
exec(...);
chdir(DRUPAL_ROOT);

rename($temp, 'public://mydir');

According to documentation as of PHP 5.0.0 rename() works with "some" url wrappers. At least that's better then nothing...not sure if there is a different move function that works will all.

@boombatower according to #21 public is always local. So you can just use public://myfile public://mydir/myfile etc? Maybe you can point to particular lines of code if something seems problematic?

chdir('public://'); does not work so we need public:// evaluated to real path.

Interesting - I think streams in general have no idea of current directory, so the user must keep track of it.

The docs could list some commonly-used functions that require the drupal_realpath('public://...') notation. This would include chdir(), imagepng(), etc. (by the way many other image functions e.g. getimagesize() do work with stream wrappers).

@ufku: I just tested with some sample files

 drupal_add_js('public://test.js');
drupal_add_css('public://test.css'); 

and it appears that drupal_add_css() and drupal_add_js() do support public:// file paths.

Also, I added some documentation at http://drupal.org/update/modules/6/7#file_directory_path please review it.

+++ modules/image/image.test
@@ -334,8 +334,7 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
   function getImageCount($style) {
-    $directory = file_directory_path() . '/styles/' . $style['name'];
-    return count(file_scan_directory($directory, '/.*/'));
+    return count(file_scan_directory('public://styles/' . $style['name'], '/.*/'));

@@ -482,7 +481,7 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
     // Confirm the style directory has been removed.
-    $directory = file_directory_path() . '/styles/' . $style_name;
+    $directory = file_default_scheme() . '://styles/' . $style_name;

(and elsewhere) The various Image module hunks in this patch are inconsistently using either 'public' or the default scheme. They should be consistently using one or the other (rather the default than a hard-coded public scheme).

+++ modules/simpletest/tests/file.test
@@ -899,6 +897,7 @@ class FileDirectoryTest extends FileTestCase {
       // Make directory read only.
+      debug($directory);

?

Powered by Dreditor.

One thing about image.module, I don't think it will work if the default scheme is non-local and you're using the gd image toolkit, because image_gd_save() won't be able to save to a non-local location. But it could work if someone made an image toolkit that supports stream wrappers. Maybe we need to check if the default scheme is compatible with the toolkit..

And/or as suggested provide a .info file flag or what not for requiring public:// to be local.

public will always be local. the question is, if file_default_scheme is local. Image module should theoretically support non-local default schemes, it's just the image.gd.inc toolkit that cannot.

FYI I just found an issue for the image.gd.inc remote stream bug: #696150: image_gd_save() does not support remote stream wrappers

changing the tag to new tagging scheme.

What's the status of the update guide section added in #31 - does it need to be edited?

@jhodgdon, #38, I think it's probably OK. I added a reference to the appropriate handbook page just now.

http://drupal.org/update/modules/6/7#file_directory_path