Problem/Motivation

The drupal_realpath() function was originally written to ease porting of 6.x code to use 7.x stream wrappers. However, it assumes that every URI may be resolved to an absolute local filesystem path, and this assumption fails when stream wrappers are used to support remote file storage. Remote stream wrappers may implement the realpath() method by always returning FALSE. The use of drupal_realpath() is therefore discouraged, and may even be deprecated.

Proposed resolution

The documentation for the drupal_realpath() should be rewritten to

  1. Explain why it should generally not be used.
  2. Point to other functionality that should be used in most cases.
  3. Point to at least one legitimate use case.

Remaining tasks

A patch has been submitted to achieve objective #1. It lacks a description of how to replace current use of drupal_realpath() with acceptable alternatives, and it lacks a legitimate use case.

User interface changes

None.

API changes

An API change node should be written to explain that the use of drupal_realpath() is now discouraged or deprecated.

Original report by Damien Tournoud

drupal_realpath() should generally not be used, as it is only meaningful on local filesystems (see #1083982: Fix support for remote streamwrappers for the full discussion). We should extend its documentation to make that fact clear.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Status: Active » Postponed (maintainer needs more info)

Maybe you could fix the drupal_realpath() doc on that other issue? Or point to a specific comment where the doc fix is described? Or describe it here? Most of that issue seems to deal with something else, and not being ultra-familiar with the file/stream API, I don't know what the doc fix needs to be.

catch’s picture

Component: documentation » file system
Category: bug » task
Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Active

Re-opening this, we need to add proper docs for drupal_realpath(), so that contrib modules use it correctly, and core patches don't revert #1083982: Fix support for remote streamwrappers. Since that was a nasty bug, this can be a major task so it doesn't get lost.

jhodgdon’s picture

Status: Active » Postponed (maintainer needs more info)

Why is fixing the doc not part of that other issue, then? Normally, a patch that changes code should also update doc...

If it's going to be here, can you please explain what "proper docs" need to be added, or point to a comment on the other issue that explains it?

catch’s picture

Status: Postponed (maintainer needs more info) » Active

Well the patch went in without updating docs, so it is too late to do there IMO. I personally find it very confusing when we re-open issues for docs after commit when there is no indication in the issue tracker that it's been committed already, and Damien already opened this one. Most of the contents of the docs are going to 'don't use it because it doesn't work for anything except local stream wrappers' and hopefully a use case for when it actually should be used.

jhodgdon’s picture

OK, then can someone please explain what docs are needed? I don't know how to write the doc.

pillarsdotnet’s picture

Issue tags: -Novice

Here's a use-case: #1113588: Provide find_conf_path() function by re-factoring reusable code from existing conf_path() function.
EDIT: Retracted. The patch in that issue used realpath(), not drupal_realpath(), and even that bit has been recently removed.

jhodgdon’s picture

Status: Active » Postponed (maintainer needs more info)

That does not answer my question of what should go into the docs. That issue has 44 comments and no summary.

Which function(s) are you saying need different documentation -- just drupal_realpath()?
And what needs to be updated in its documentation?

Here is the doc page for reference:
http://api.drupal.org/api/drupal/includes--file.inc/function/drupal_real...

Damien Tournoud’s picture

Status: Postponed (maintainer needs more info) » Active
Issue tags: +Novice

The doc fix is described in the OP here:

- Don't use it
- Only necessary when you want direct access to a file and you know that the stream wrapper you are using is local

Not sure what more is necessary except doing a patch, which is what this issue is for.

pillarsdotnet’s picture

Status: Active » Needs review
Issue tags: +Novice
FileSize
1.68 KB

Here's a patch that changes the docblock to the following:

/**
 * Returns the absolute path of a file or directory
 *
 * This function was originally written to ease the conversion of 6.x code
 * to use 7.x stream wrappers.  However, stream wrappers may be used to
 * support remote file storage, which is incompatible with the assumption
 * that every URI may be resolved to an absolute path on the local filesystem.
 * Therefore, the use of drupal_realpath() is now discouraged, and it is
 * slowly being removed from core functions where possible.
 *
 * Only use this function if you know that the stream wrapper in the URI
 * uses the local file system, and you need to pass an absolute path to
 * a function that is incompatible with stream URIs.
 *
 * @see http://php.net/manual/en/function.realpath.php
 *
 * Compatibility: normal paths and stream wrappers.
 * @see http://drupal.org/node/515192
 *
 * @param $uri
 *   A string containing the URI to verify.
 *
 * @return
 *   The absolute pathname, or FALSE on failure.
 *
 * @see realpath()
 * @ingroup php_wrappers
 */
skwashd’s picture

Status: Needs review » Needs work

The new docs are lot clearer. I have a few suggested changes.

+++ b/includes/file.incundefined
@@ -2194,11 +2194,16 @@ function drupal_unlink($uri, $context = NULL) {
  * Returns the absolute path of a file or directory

The summary could make this clearer: "Returns the absolute path of a file or directory on the local filesystem"?

+++ b/includes/file.incundefined
@@ -2194,11 +2194,16 @@ function drupal_unlink($uri, $context = NULL) {
- * to the registered wrapper for handling. If the URI does not contain a
- * scheme or the wrapper implementation does not implement realpath, then
- * FALSE will be returned.

Should retain info that "not implemented" is considered a failure and returns false in @return or somewhere else in docs.

+++ b/includes/file.incundefined
@@ -2194,11 +2194,16 @@ function drupal_unlink($uri, $context = NULL) {
  * @see http://php.net/manual/en/function.realpath.php

Move to bottom and replace "@see realpath()".

10 days to next Drupal core point release.

pillarsdotnet’s picture

Re-rolled, taking suggestions from #10 into account.

Should retain info that "not implemented" is considered a failure and returns false in @return or somewhere else in docs.

Actually, the original wording is incorrect. Remote stream wrappers do implement the realpath function -- by always returning FALSE.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
2.16 KB

Re-rolled with whitespace corrections and fewer indefinite pronouns.

skwashd’s picture

+++ b/includes/file.incundefined
@@ -2192,15 +2192,19 @@ function drupal_unlink($uri, $context = NULL) {
+ * realpath method by always returning FALSE. Therefore, the use of
+ * drupal_realpath() is now discouraged, and it is slowly being removed from
+ * core functions where possible.

I think it should be either "Therefore, the [...] discouraged and it is [...]" or "The [...] discouraged and is". My preference is for the latter.

Leaving it as needs review, because my English can suck sometimes and this is a nitpick.

10 days to next Drupal core point release.

pillarsdotnet’s picture

No, you're right, and the use of "now" without a time reference is possibly confusing. Re-rolled.

Revised docblock is as follows:

/**
 * Returns the absolute local filesystem path of a stream URI.
 *
 * This function was originally written to ease the conversion of 6.x code to
 * use 7.x stream wrappers. It assumes that every URI may be resolved to an
 * absolute local filesystem path. This assumption fails when stream wrappers
 * are used to support remote file storage. Remote stream wrappers implement the
 * realpath method by always returning FALSE. The use of drupal_realpath() is
 * discouraged, and is slowly being removed from core functions where possible.
 *
 * Only use this function if you know that the stream wrapper in the URI uses
 * the local file system, and you need to pass an absolute path to a function
 * that is incompatible with stream URIs.
 *
 * Compatibility: normal paths and stream wrappers.
 * @see http://drupal.org/node/515192
 *
 * @param $uri
 *   A string containing the URI to verify.
 *
 * @return
 *   The absolute pathname, or FALSE on failure.
 *
 * @see http://php.net/manual/function.realpath.php
 * @ingroup php_wrappers
 */
catch’s picture

FileTransfer is a safe bet since that is only dealing with the local filesystem (we don't allow modules anywhere else).

Most of the file_*() functions are now only using drupal_realpath() in case of error conditions, for example:

http://api.drupal.org/api/drupal/includes--file.inc/function/file_unmana...

The drupal_realpath() call is only to get the location for the error message - however I would struggle to find a way to document that it should only be used to generate pretty error messages in those contexts so possibly not a good example.

pillarsdotnet’s picture

@catch -- I don't understand why checkPath() uses drupal_realpath() instead of realpath().

The code says:

  $full_path = drupal_realpath(substr($this->chroot . $path, 0, strlen($full_jail)));

I see that $this->chroot is set by findChroot() which starts with __FILE__ and trims down from there.

So the argument to drupal_realpath() in this case is guaranteed not to be a stream wrapper URI.

It appears to me that the only reason checkPath is not using realpath() is that nobody wanted to write a justifying comment.

catch’s picture

@pillarsdotnet, that sounds right, worth rolling a patch for to remove it from there too then.

If we look through these and end up with either cases where it should be just realpath(), or is only being used for error messages, then we could go a step further here and mark drupal_realpath() deprecated maybe?

pillarsdotnet’s picture

Already marked "discouraged" -- dunno what the proper procedure is for marking something "deprecated" -- can you point to a useful example?

catch’s picture

Just @deprecated I think, there's not really a proper pattern for this yet.

pillarsdotnet’s picture

pillarsdotnet’s picture

Searched for the word "deprecated" in 8.x source and found an example to emulate.

pillarsdotnet’s picture

pillarsdotnet’s picture

Untagging. This is not Novice when at every step I need the advice of a core committer to continue.

pillarsdotnet’s picture

Issue tags: -Novice

Updated summary using the issue summary template.

pillarsdotnet’s picture

Issue summary: View changes

Using issue summary template.

pillarsdotnet’s picture

Issue summary: View changes

Missed a link. (oh, my!)

pillarsdotnet’s picture

Issue summary: View changes

More links.

jhodgdon’s picture

RE #22 - I think this is pretty good, and maybe we don't need a usage example.

One thing confused me though:

 * Compatibility: normal paths and stream wrappers.

What does this mean? Is this saying that the input can be a file path or a stream wrapper? If so, I think it should go into the @param instead of being on its own line -- normally, that is the place to document information specific to parameters. And why does it say "normal" -- what is a normal path as opposed to a non-normal path?

pillarsdotnet’s picture

What does this mean?

I think it means that the argument can be a local filesystem path or a stream wrapper.

And why does it say "normal" -- what is a normal path as opposed to a non-normal path?

Think "normal" path versus "Drupal" path. A Drupal path is relative to the Drupal root. A normal path is relative to the current working directory, or (in PHP) to the include_path.

pillarsdotnet’s picture

Re-rolled with (hopefully) more clarity.

pillarsdotnet’s picture

Inserted the word "may" -- an important distinction.

jhodgdon’s picture

Status: Needs review » Needs work

OK. Since I didn't know what it meant from reading the doc (and I think I'm a fairly informed PHP/Drupal programmer), maybe that information could get into the patch (and please put it into the @param)? :)

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

Grrr!!! For real, this time.

jhodgdon’s picture

Status: Needs review » Needs work
+ *   A string either a stream wrapper URI or a filesystem path, possibly
+ *   including one or more symbolic links.

I think this needs some punctuation, or maybe rewrite it as:

@param string $uri
A stream wrapper URI or a file system path, possibly including one or more symbolic links.

pillarsdotnet’s picture

As usual, you are correct. I had inadvertently removed the word "containing" but I like your version better. Re-rolled.

EDIT: None of the other @param $uri lines in this file specify that $uri should be a string. If we change this one, we should change the rest for consistency.

pillarsdotnet’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Having data types in @param statements is a relatively new doc standard. There is no real reason why it shouldn't be adopted here, just because it hasn't been adopted everywhere else yet.

Anyway, the patch looks fine to me. Should it be backported to 7.x too?

pillarsdotnet’s picture

Issue tags: +Needs backport to D7

Should it be backported to 7.x too?

Yes, since the parent issue was also backported to 7.x.

Adding the tag, but the patch applies cleanly to both 8.x and 7.x.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

pillarsdotnet’s picture

@jhodgdon:

Having data types in @param statements is a relatively new doc standard. There is no real reason why it shouldn't be adopted here, just because it hasn't been adopted everywhere else yet.

Posted #1290258: The data type should be explicitly stated for all Drupal core @param and @return documentation.

Status: Fixed » Closed (fixed)

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

Sheldon Rampton’s picture

I just found this thread after reading that drupal_realpath is deprecated on its documentation page:

http://api.drupal.org/api/drupal/includes--file.inc/group/file/7

If I'm understanding this thread correctly, there is no viable alternative to drupal_realpath in cases where there is a need to pass an absolute path on the local file system to a function. I think there are legitimate cases where such a need exists, which means that drupal_realpath should not be deprecated. Instead, it should simply be documented to make clear the limits of when it should and should not be used.

As an example of a use case where there is a need to pass an absolute path, I'm developing a module that will let site users upload a file using a managed_file field in a custom form created using the Forms API. Upon form submission, the uploaded file along with other form submission results should be emailed to a recipient specified by the module. To generate the email, I plan to use Allie Micka's mimemail module, which requires me to supply the real path to the file as a parameter when constructing the MIME email.

jbrown’s picture

Status: Closed (fixed) » Needs review
FileSize
2.11 KB

Yes - the documentation is incorrect.

drupal_realpath() is not deprecated, has valid uses and is used in core.

jhodgdon’s picture

Hm. I agree with these two assessments that this function is used in core, and should not be marked as deprecated... but apparently the maintainers of the File System do not. We need someone to weigh in on this...

The patch as it stands, if the content is acceptable to the File System maintainers, also needs some rewriting:
- e.g. is misused. Please write out "for example" instead and see if it makes sense, and then punctuate correctly.
- It changes "filesystem path" to "filepath" -- I think "filesystem path" is clearer.

Sheldon Rampton’s picture

jhodgdon: Hm, well, I've already weighed in with my thoughts on the matter. I guess if the maintainers of the file system feel that this function should be deprecated, I would appreciate an explanation of WHY they think it should be deprecated. I don't think the reasoning presented in this ticket justifies deprecating it. The start to this ticket states:

The drupal_realpath() function was originally written to ease porting of 6.x code to use 7.x stream wrappers. However, it assumes that every URI may be resolved to an absolute local filesystem path, and this assumption fails when stream wrappers are used to support remote file storage. Remote stream wrappers may implement the realpath() method by always returning FALSE. The use of drupal_realpath() is therefore discouraged, and may even be deprecated.

First of all, the drupal_realpath() function doesn't "assume" anything. It's a computer function, not a sentient life form, so it is incapable of making assumptions. I guess the PEOPLE who wrote that function assumed that's how it would be used, but in fact that's not how I used it and how other people are using it. In my case, I actually used it to look up the real path to my uploaded file, which seems to me like a reasonable way to use a function with the name drupal_realpath. As I explained above in comment #40, I needed the real path in order to get file attachments working with emails that my website automatically generates. I didn't need to wrap anything in stream wrappers. I just needed the path to my file. If drupal_realpath() isn't the way to accomplish that, what is? You can't just remove functionality from Drupal without providing an alternative.

Right now the documentation for drupal_realpath() states that the function is deprecated, but it doesn't provide enough information to enable a developer to write an alternative so we CAN remove it from our modules. The documentation simply points to the PHP realpath() function's documentation and to an abstract method DrupalStreamWrapperInterface::realpath() which says it is an "Implementation placeholder. PHP's realpath() does not support stream wrappers. We provide this as a default so that individual wrappers may implement their own solutions." Sorry, but this documentation is completely unhelpful as a way of explaining how to create an alternative to drupal_realpath(). As a developer who is actually trying to get work done, am I supposed to just stumble across this "implementation placeholder" and then start trying to write my own wrapper so I can implement my own solution? How do I write a wrapper?

The file system maintainers will probably read the above paragraph and think, "Oh, Sheldon, that's silly. Wrappers already exist. You don't have to write your own. The subclasses of DrupalStreamWrapperInterface() have already implemented the solution you need." OK, but THAT'S NOT WHAT THE DOCUMENTATION SAYS. When I'm actually trying to get work done in Drupal I shouldn't have to trace back and forth through stream wrapper classes and subclasses and puzzle out what they're doing just to figure out how to get the real path to a file. Drupal is supposed to be making my work as a developer easier, not harder. Either write real documentation for us, or leave the drupal_realpath() function alone and don't deprecate it. If you want to warn people against using it in contexts where people need stream wrappers, fine. Warn them. But don't tell people to stop using drupal_realpath() until you've written real documentation for an alternative.

To put this another way, if the file system maintainers want to remove drupal_realpath() from Drupal 8, they need to provide an instruction for how to convert existing D7 modules so that our D8 versions of those modules will work without using the drupal_realpath() function. I imagine that documentation would say something such as the following:

The drupal_realpath() function has been eliminated. In its place, you should do the following:

Drupal 7 version:
$path = drupal_realpath($uri);

Drupal 8 version:

  if ($wrapper = file_stream_wrapper_get_instance_by_uri($uri)) {
    $path = $wrapper->realpath();
  }
  elseif (!empty($uri)) {
    $path = realpath($uri);
  }
  else {
    // failure handling in cases where $uri is empty
  }

An instruction along those lines would enable me to stop using the drupal_realpath() function, but it does so at the cost of forcing me to write multiple lines of code to do the same thing that the drupal_realpath() function lets me do now by just calling the function. I don't think that's a good idea. It makes it harder to perform what ought to be a simple task. It forces me to write longer, messier code that will be harder to maintain.

Bottom line: If the guys who wrote this function didn't want it to be used to get the real path to files, they shouldn't have created it and named it drupal_realpath() in the first place. Maybe they should have named it drupal_ease_porting_stream_wrapper_hack_dont_use_this_function_we_didnt_really_mean_it().

jbrown’s picture

FileSize
2.11 KB

I think the desire to make it deprecated came out of the fact that it was being used inappropriately in so many places: #1083982: Fix support for remote streamwrappers. What is important is that URIs are not resolved before passing to a function that is capable of handling URIs, otherwise the advantage of URIs over filepaths is lost.

An example of a valid use is in image_gd_save().

The reason my patch changes "filesystem path" to "filepath" is that "filepath" is the established term in core.

Sheldon Rampton’s picture

@jbrown: OK, well, "deprecated" means "a command or statement in the language that is going to be made invalid or obsolete in future versions." It does not mean, "a command or statement that is still being used and which we just wish people would learn to use correctly." If it is still being used in core, and there are still correct ways to use it, then that means it cannot be considered deprecated.

sun’s picture

@Damien Tournoud: Are you down with the latest patch?

Damien Tournoud’s picture

Status: Needs review » Needs work

There are very very few good reasons to use drupal_realpath(). There definitely *are* good reasons (including using libraries that only accept local files like in image_gd_save() as indicated above, but in that case you need to *also* handle the creation of a temporary file), but most of the module being ported to Drupal 7 have no good reasons to use them.

As a consequence, the language in #44 is inappropriate. This function is not deprecated, but its usage needs to be strongly discouraged.

jbrown’s picture

@Damien Tournoud the comment in the patch states clearly that normally the function should not be used - if you want stronger language can you please supply it?

drupal_realpath() is not evil - it just needs to be used when appropriate.

jbrown’s picture

Status: Needs work » Needs review

#44: drupal_realpath-docs.patch queued for re-testing.

tim.plunkett’s picture

Assigned: Unassigned » Damien Tournoud
FileSize
1.84 KB

How's this for a compromise?

Damien Tournoud’s picture

I still think we need to remove this function.

We should replace it with something that makes a temporary copy of the file, and an API to either "commit" the temporary back to the storage or just "revert" by removing the copy.

Something like:

$copy = drupal_tempcopy("s3://xxxx/xxxx/xxxx.jpg");

// Returns an absolute path to the copy.
$copy->getLocalPath();

// Returns a handle to the copy.
$handle = $copy->open('w');

if (no errors) {
  // Performs either:
  //  - On a local filesystem: a standard flush + fsync + rename (ie. an atomic replace)
  //  - On a remote filesystem: a lock + write + unlock
  $copy->commit();
}
else {
  // Just remove the copy
  $copy->rollback();
}
Sheldon Rampton’s picture

@Damien Tournoud: My use case is satisfied as long as a well-documented system exists whereby developers can write code that gets a local path and a handle to a file (or a copy of that file) after it has been uploaded via a Drupal form submission. The specific use case that brought me to this ticket thread was:

  • I was asked to build a website that would let jobseekers apply for jobs by filling out a form (created using the Forms API) through which jobseekers would upload their resumes.
  • Upon uploading, the website would email the uploaded resume files (e.g., PDFs, MS Word docs) to a list of potential employers and then delete the uploaded files.

In order to generate the emails, I needed a local path to an actual file, which led me to drupal_realpath(). It sounds like your proposal would give me a reasonably easy way to get a local path to an actual file, so I could live with it. But what about use cases where someone wants a real path to a file so they can write to it? Writing to a copy of the file won't work.

I appreciate the value in abstracting away from the particulars of a file system to use stream wrappers, but it seems to me that in practice there are going to be cases where people still need to get a real filepath. There is a lot of PHP code (not necessarily Drupal code, but PHP libraries, etc.) that takes a filepath as a parameter and does something to file.

Anyway, I think that your proposal could satisfy my use case. Maybe we can adopt Tim Plunkett's patch #50 for the time being, and start a separate ticket to implement your proposal?

kerasai’s picture

#50: realpath-1201024-50.patch queued for re-testing.

alfaguru’s picture

Yes, @Sheldon Rampton, @Damien Tournod, I have a similar use case where I am integrating with an API to which I have to pass a file to be posted. If I use the stream wrapper uri, it throws an exception, if I use drupal_realpath to get the actual temporary file path, it's happy.

So my vote would be not to deprecate this function until there's a proper alternative.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

IMHO, let's RTBC #50 until it gets forgotten and move #51 to another issue.

jbrown’s picture

I agree - #50 still applies. #51 is separate issue

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I'm not really excited about the new first line of the documentation, which says "Resolves the absolute filepath...". I don't know what "resolves" means here, and I had to read all the way down to the return value documentation to figure out what the function did. Can it be written in a way that tells me what the function actually does?

jbrown’s picture

"resolves" is entirely the correct term. The function should really be called resolve_uri(). It resolves a URI into the filepath it represents in the same way that DNS resolves a domain name into the IP address it represents.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

ok.

Damien Tournoud’s picture

Assigned: Damien Tournoud » Unassigned

Ok for me too.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Docs look good to me. Committed/pushed to 8.x.

Moving to 7.x for backport.

jbrown’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.9 KB
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/19e85c0

I think that means this issue can be closed (and everything else handled in followups) but reopen if I'm wrong.

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

Anonymous’s picture

Issue summary: View changes

Remote stream wrappers don't *have* to return FALSE for realpath().