[Push #2] pathfilter for files directory

Could you create a proper patch? http://drupal.org/patch

+1 for this patch. It will definitely make things easier for my clients (and me). Works fine in the June 18 release of the module. Also only tested on clean URL 5.1 site.

By the way, if you get the TinyMCE integration working properly, I'd be very interested in that too.

One problem with this patch is that it prepends base_path() to the replacement text. It is not always the case that the files directory lives inside the drupal directory.

A second shortcoming is that it only works with files in files directory. If you wanted to link to say, an image in the misc directory or a themes directory, it would not work.

The patch I'm attaching is very simple, and allows "internal:..." to link to either an internal path, or a file within the drupal directory. So for example "internal:files/upload.gif" would resolve correctly, even if clean URLs is disabled.

One drawback with my patch is that if the files directory is moved, the links would break. But personally I can live with that.

+1 and subscribing.... i need this feature ;) (altrough i think this feature is incompatible with private download method)

-1 for pointing to themes directory in content, you have the l() function in your themes for example, also content should not be dependant on theme

@Dave Cohen

One drawback with my patch is that if the files directory is moved, the links would break. But personally I can live with that.

not if you use file_directory_path()

not if you use file_directory_path()

True that the code could check file_exists($1) and if not found then check file_exists(file_directory_path() . $1), and then if still not found fall back to url(...). That's a worthwhile improvement.

However, because Drupal caches the results of most filters, the links would still be broken until the filter cache is refreshed.

I'm marking this as code needs review (patch #5) because this has been here a long time and I doubt anyone has tried it.

However, because Drupal caches the results of most filters, the links would still be broken until the filter cache is refreshed.

you probaly can fix that automagically by flushing the cache when you're changing file_directory_path() trough the admin-panel by using the right hook.

One drawback with my patch is that if the files directory is moved, the links would break. But personally I can live with that.

some can, some can't, however, since there is a better way, we should follow that road! (less breakage)

secondly, in my opinion, there should be a distinction between "physical" files, and "virtual" nodes.
the files: operator is perfect for this, while also keeping development open for more options we don't think about now (what about images: or add-ins from other modules?)

also, i think the file exists check is too much of a performance problem.

putting this back to code needs work, after this small review.

alexanderpas,

I noticed you also gave a +1 to this feature: http://drupal.org/node/148300. One advantage to using just "internal:" and not introducing "files:" is that that feature would work equally for internal paths and files.

Also, an advantage to not using file_directory_path() is that files in misc/ (or any directory) can be referenced this way.

Finally, file_exists() introduces overhead as you point out, but only when the filter is processed. Usually, the result of the processing is cached. I suspect the overhead of file_exists pales in comparison to the regular expressions being used.

I think you have good points. There are trade-offs with either approach; that's all I'm trying to say here.

/misc or any other drupal owned path besides file_directory_path() shoudn't be referenced inside nodes in my opinion!

there is NO REASON (prove me wrong please) to reference any physical files outside file_directory_path() inside nodes! (unless you have a bad file structure)

Also: limiting internal: to drupal owned paths, and files: to file_directory_path() keeps this module managing only content that is part of your drupal installation.

Yes, i gave the other issue also a +1 as we rerally need a method that automagically change absolute links into internal paths!

there is NO REASON (prove me wrong please) to reference any physical files outside file_directory_path() inside nodes! (unless you have a bad file structure)

I have used third party modules which give more control over where some files are uploaded. In some cases I use drupal's file directory for files which require access control and other directories for files that don't.

I'm not very attached to either of the methods we're talking about here. I say +1 to your approach. If I find I need to continue using my approach, I'll keep using a patched module.

+1 for files: = file_directory_path()

This is for D6, i've made some changes to the OP's code as there is really no problem with this implementation.

The file_directory_path() is wrapped in url() to keep it consistent.
Changed regexp for files to allow for dirs i.e, files:images/file.jpg (for linking). Works with Markdown too.

case 'process':
  $absolute = (variable_get('pathfilter_link_type', 'absolute') == 'absolute' ? 'TRUE' : 'FALSE');

  $patterns = array(
    '/"internal:([^"#\?]+)\??([^"#]+)?#?([^"]+)?"/e',
    '/"files:([\w\/.-]+)"/e',
  );
  $replacement = array(
    "'\"'. url('$1', array('query' => '$2' ? '$2' : NULL, 'fragment' => '$3' ? '$3' : NULL, 'absolute' => ". $absolute .")) .'\"'",
    "'\"'. url(file_directory_path(), array('absolute' => ". $absolute .")) .'/$1\"'",
   );

I don't trust that $1 outside the url, shouldn't that be concatenated inside the url() together with file_directory_path()
also, the patch changes the name...

also, I personally think we should keep the pattern and replacement together... they form a pair, also, this makes extending this function easier.
I've added those things to this patch.

#15 nice one. There was some strange behavior for me with $1 inside url() I can't remember what it was, maybe encoding but certainly should be as you suggested. Review to follow.

Alexander's #15 patch tested working, albeit the files pattern still does not allow for whitespace in filenames -- care?

I'm using this with markdown.module for image links, it's awesome:

[![Contact Us](files:images/contact.jpg)](internal:contact)

whitespaces in filenames are dirty business... as they can't truely exist on the internet... therefor %20 or + or something like that get's used

I'm not putting my own patch on RTBC... I'm leaving that to someone else. (next reviewer maybe)

#15 RTBC

+1 This would be a fantastic feature!

I implemented my own using the following process step (Drupal 5):

<?php

        // The actual filtering is performed here. The supplied text should be
        // returned, once any necessary substitutions have taken place.
        case 'process':
          return preg_replace('/"file:([^"]+)"/e', "'\"'. file_create_url('$1') .'\"'", $text);

?>

Just one question: Shouldn't file_create_url be used instead of url? It doesn't take into account absolute/relative but it is how Drupal does it.

file_create_url seems to make this compatible with the private download method, therefor marking this as code needs work.

I'm aware of the double url() call, yet i don't see an easy way to fix that, without losing the relative url functionality.

#402296: Call for Action: Get Pathfilter Additional Functionality in before D7 makes pathfilter obsolete (Pathfilter in Core???)

Try with this patch, made against the latest code in CVS. I have basically used the code from #22, with a couple of modifications. The main change is to make this work with preg_replace_callback() which I am now using in place of preg_replace as it a) allows for some much more advanced rewrites (required for #408086: Use correct alias for translated nodes.) and b) is a lot faster.

I have also added a few tests for the file:filepath style filter, but they are still quite quite limited.

oops... last patch had 2 failing tests. Here is an updated one with all tests passing:

some remarks while i'm sleeping

+  switch ($matches[2]) {
+    case 'internal':
+      return _pathfilter_process_internal($matches);
+      break;
+      
+    case 'files':
+      return _pathfilter_process_files($matches);
+  }

how about:

if (function_exists('_pathfilter_process_' . $matches[2])) {
  _pathfilter_process_$matches[2]($matches);
}

also, data found beyond column 80 ;)

I'm not keen on your alternate syntax there. Shorter, yes, but not half as clear. Data is found beyond column 80 throughout the Drupal Core! Anyway, I have done a little more cleaning of the code, added a little documentation in the filter tips and committed this to CVS. Perhaps this will get some more testing eyes on it. Oh I also altered the regex for detecting a files: match as ([\w\/.-]+) was just too restrictive (file names can contain spaces amongst other things). I used ([^"\']+) instead. Perhaps that is not restrictive enough though?

Needs porting to Drupal 5.

Backported to D5