The documentation for the DrupalLocalStreamWrapper::getLocalPath() function says:

   * Return the local filesystem path.

This description is insufficient, imprecise, and therefore misleading. The (undocumented) return value is explicitly set to FALSE in certain cases.

See: #1008402: Allow the use of symlinks within the files directory.
And: #155781: "realpath" check breaks symbolic links in file directory

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pillarsdotnet’s picture

Issue summary: View changes

added a line break and some quote marks for clarity.

pillarsdotnet’s picture

Issue summary: View changes

Make the example doc line up with the original.

pillarsdotnet’s picture

Issue summary: View changes

wordsmithing.

jhodgdon’s picture

Issue tags: +Needs backport to D7

I don't think realpath() should be used as a noun in that suggested sentence, and "of a valid URI" should have URI in all caps (it's an acronym). Also, "or FALSE otherwise"... otherwise? That should explain under what conditions in the parameters FALSE is returned. No, actually that should go in the (currently nonexistent) @return, not in the first line summary.

Anyway, looks like a good thing to fix up!

pillarsdotnet’s picture

Status: Active » Needs review
FileSize
1.36 KB

Here's a first-draft patch.

pillarsdotnet’s picture

Slightly improved; capitalized URI for consistency with the rest of the file.

jhodgdon’s picture

Status: Needs review » Needs work

I don't think the return docs are quite accurate here. If $uri is not given, the function sets it from $this->uri, which returns whatever has been set with the ::setURI() method in this instance of the class. So I don't think it's accurate to say that if the $uri parameter is not set, it returns the path to the repository? But I'm not an expert in stream wrappers, so maybe I'm misunderstanding something.

Also, looking at what the function does, it could return the path to the file, or a path to the directory the file will be going into, and I don't think the @return is saying this?

And what is a "canonical file system path" anyway? Is this something I'm just supposed to know, or could it be explained?

pillarsdotnet’s picture

Status: Needs work » Needs review

The DrupalStreamWrapperInterface::setUri() function is used to assign a base URI to the stream wrapper interface instance. Its documentation could also use some improvement.

Neither the original documentation nor my suggested replacement assume that $uri points to a regular file, as distinct from a directory or other type of file. But if the $uri parameter is supplied and valid, the purpose of this function is to convert it from a stream wrapper URI to a canonical filesystem path.

canonical

Begins at the filesystem root rather than the current working directory, and contains no symbolic links in any intermediate path component.

filesystem

Does not explicitly reference a network resource, and thus can be used without stream wrapper support.

path

May contain two or more path components separated by the slash character.

See the realpath() documentation.

A stream wrapper URI may be considered a path, but not a filesystem path.

A path may resolve to any kind of file, including a directory, socket, fifo, block or character device, etc. A directory is a special kind of file. Precise documentation refers to "regular files" as distinct from directories etc. Saying "files and directories" is as regrettably common and imprecise as saying "fruits and vegetables."

A Drupal path that does not contain a scheme:// part may be used as a (relative) filesystem path, but it is not canonical as it does not begin with a slash character.

A path that traverses symbolic links is not canonical. For instance, if /tmp is a symbolic link that points to /var/tmp, then /var/tmp/foo would be canonical, while /tmp/foo would not be canonical.

A path that contains directory traversal operators such as "/../" or "/~/" is not canonical.

I don't believe that an introduction to filesystem semantics should be included in this documentation block, although it may be appropriate to @see one. Does drupal.org contain such a resource?

EDIT: The documentation for file_create_url() contains helpful information.

jhodgdon’s picture

In general documentation should avoid jargon and everything in it should be be clear to most readers, right? So:

a) If the function doesn't support files, but only directories, can we make that clear in the documentation? I didn't know that, and some random person who landed on that api.drupal.org page via a Google search or something might not either, I think? The one-line doc says "Returns the canonical path of the URI...", and I thought that "URI" could refer to a file URI -- actually the way the function works and the inline comments implies that? I think?

b) I agree we don't need an intro to file system semantics, but I also didn't know what a "canonical" file system path meant. Maybe I am unique and most people reading this function would? If you think so, leave the term in. If not, please replace it with a non-jargon term. Obviously, a 20-line explanation is not the right thing, but maybe just calling it a "file system path" and leaving out the word canonical (which doesn't seem to add any meaning to me -- again not an expert in this stuff)?

Also, my comment in #4 still stands about the @return -- after reading the function, I don't think the @return in the patch reflects what the function actually does. If you think I'm wrong, please recruit someone more knowledgeable about the file API to review the patch, and I'll happily refrain from reviewing that part for accuracy. Thanks!

pillarsdotnet’s picture

Status: Needs review » Needs work

a) The function supports both directories and other types of files.

b) I do not know how to replace the word "canonical" with "non-jargon terms" that fit in a single 80-character summary line. As I explained above, the term "file system path" is insufficiently precise. We need to do one of the following:

  1. Use the phrase canonical filesystem path.
  2. Use something else that means the same thing.
  3. Explain that the result is passed through the php realpath() function (which is what makes it canonical).

You have rejected my attempts at #1 and #3, and I don't know how to do #2 in fewer than 80 characters.

pillarsdotnet’s picture

See the DrupalStreamWrapperInterface::realpath() documentation, which also assumes that the reader knows what "canonical" means.

Revised for consistency with the other function documentation.

jhodgdon’s picture

OK on canonical then.

A few items to fix:

a) "Returns canonical, absolute path of" ==> needs "the" -- "Returns the canonical..."

b)

+   *   If $uri is not set, returns the path previously set by the
+   *   DrupalStreamWrapperInterface::setUri() function.

I think this would be more accurate if it said "uses the URI previously set" rather than "returns the path previously set"? And maybe this should go into the $uri @param section instead anyway?

c) If the function is intended to be used with either files or directories as you stated in #7a, then the @return should state somewhere that if the URI refers to a file that does not yet exist, the path to the directory it's in is returned instead, I think? That is what I think the function body and its internal comments say is happening.

pillarsdotnet’s picture

Status: Needs work » Needs review

If $uri is not given, the function sets it from $this->uri, which returns whatever has been set with the ::setURI() method in this instance of the class. So I don't think it's accurate to say that if the $uri parameter is not set, it returns the path to the repository?

Note that the (undocumented) DrupalLocalStreamWrapper::realpath() function is merely an alias for DrupalLocalStreamWrapper::getLocalPath().

So for all implementations of the DrupalLocalStreamWrapper class, the following expressions are equivalent:

  • drupal_realpath($uri)
  • file_stream_wrapper_get_instance_by_uri($uri)->getLocalPath($uri)

And for every valid stream wrapper URI, the following expressions are equivalent:

  • file_stream_wrapper_get_instance_by_uri($uri)->getLocalPath($uri)
  • realpath(file_stream_wrapper_get_instance_by_uri($uri)->getDirectoryPath() . '/' . file_stream_wrapper_get_instance_by_uri($uri)->getTarget($uri))

Therefore, the following two expressions (for example) are equivalent:

  • file_stream_wrapper_get_instance_by_uri('public://')->getLocalPath()
  • drupal_realpath('public://')

And if drupal_realpath('public://') does not mean the same thing as "the base directory of the public file repository", then I don't know what does.

pillarsdotnet’s picture

Issue tags: +Needs committer feedback

As I review the above comment, I realize that it is too long for you to take time to read, so this issue is probably a "won't fix" unless a core filesystem maintainer can step in.

Therefore, tagging.

pillarsdotnet’s picture

I think this would be more accurate if it said "uses the URI previously set" rather than "returns the path previously set"?

You're right; my description was inaccurate. Corrected.

the @return should state somewhere that if the URI refers to a file that does not yet exist, the path to the directory it's in is returned instead, I think?

Note that realpath($path) will return FALSE if $path does not exist.

Therefore, DrupalLocalStreamWrapper::getLocalPath($uri) will return FALSE if $uri points to a file that does not exist.

So noted in the @param string $uri documentation, even though I think that the @return documentation is the more appropriate place to describe the return value.

pillarsdotnet’s picture

Better.

pillarsdotnet’s picture

Issue summary: View changes

wordsmithing.

jhodgdon’s picture

Status: Needs review » Needs work

Here is what I meant about the file name not existing remarks above. From the function code:

$realpath = realpath($path);
  if (!$realpath) {
    // This file does not yet exist.
    $realpath = realpath(dirname($path)) . '/' . basename($path);
  }

This seems to say that if the file doesn't exist, it uses the real path of the implied directory, and then appends the base name, right?

So I still don't think this is correct:

If $uri is set but not valid or points to a file that does not exist, returns FALSE.

Other than that, I think this is looking good (patch in #13)

Not sure if this still needs the "needs committer feedback" tag, as I think you figured out what I was saying and the doc is pretty close now?

pillarsdotnet’s picture

Yup; you're right. Corrected version attached.

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: -Needs committer feedback
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, that works for me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x.

One thought that I had when reviewing this patch: the PHPdoc is very technical and doesn't really help new people discover its purpose. It's a good update, but in a future update, it wouldn't hurt to explain a bit more 'why and when' this function can be used.

pillarsdotnet’s picture

@#18 by Dries on October 16, 2011 at 4:54pm:

... in a future update, it wouldn't hurt to explain a bit more 'why and when' this function can be used.

You caught me. I don't really know under what circumstances this function should be used. I only found it while researching the code path of the drupal_realpath() function, which is now deprecated anyway.

How should I go about soliciting input from the relevant maintainers?

  1. Contact Andrew Morton?
  2. Post a Drupal file system support request?
  3. Try to catch someone on IRC?
  4. Ask in the Module Development and Code forum?

Obviously posting an issue in the documentation queue is not an effective way to solicit input.

jhodgdon’s picture

Yeah, that's a good point. Filed this follow-up issue:
#1311554: Review and improve the docs for the File API

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

Anonymous’s picture

Issue summary: View changes

Corrections.