Import files in public://feeds/ vs. private://feeds/

I almost have the same problem. What I want to do is to import a csv file from private://.
Edit: My bad, this works for me.

I think public:// is hardcoded in FeedsFileFetcher.inc

It is not hardcoded (or at least it isn't now). It is retrieved from file_default_scheme(), which is set via the file system configuration in the UI.

So to enable it to automatically look in your private directory you need to change your default filesystem setting.

Although, I am not sure what the rationale is for using that as the *only* place you can place the file. The code comments state that you could potentially upload any file from the server, but I would imagine that using private:// or public:// stream wrappers should solve that issue?

Perhaps file_stream_wrapper_valid_scheme() could be used instead? This would validate that the scheme is supported. Otherwise, a simple check on the uri file_uri_scheme matching public or private could be used?

Re-read the issue. I see what you are saying.

I had the same need. The attached patch exposes new configuration options for the upload directory. It also allows the administrator to restrict the url-schemes available for users for direct import.

1: I don't have any idea how I'd implement a module-wide setting for the file fetcher class. Can you give an example of another fetcher class which does that?

2: The validation code is very messy. IMHO it would be best to split up this class in one for file-uploads and another one for the server directory import batch. They really do not share much code. Nevertheless, patch attached reenabling the behaviour that the last file is used when none is uploaded.

HI.

Here's a replacement module to fix the "public" issue.

Please try.

jmariani@git.drupal.org:sandbox/jmariani/1394414.git

You can view jmariani's sandbox here http://drupal.org/sandbox/jmariani/1394414. Unfortunately, as the module only contains the .info file and the description just advises a simple change to the code I don't really see much point to it.

I have tried the changes proposed by znerol and everything works perfectly. Thanks for the work you have done here znerol! I couldn't get his patches to apply to my code so I applied both of his patches by hand and created a new patch that combines the two (attached).

I would very much like to see this in the next version of Feeds :)

I use #5 and #7 in production on top of 7.x-2.0-alpha5. You can apply those patches using patch parameter -p1:

cd /tmp
drush dl feeds
cd feeds

wget http://drupal.org/files/0001-Configurable-upload-directory-for-FeedsFileFetcher.patch

wget http://drupal.org/files/0002-FileFetcher-Use-last-file-if-none-was-uploaded.patch

cat 000*.patch | patch -p1

I set this to RTBC. Would be great if I won't have to patch new releases...

I found an issue with the code when I tried to migrate this code from one server to another. A fatal error occurs when the file wrapper can't be found due to a misconfiguration between the module and core. Essentially, with the above patch in place Feeds thinks a wrapper called "private" should exist, but Drupal can't find this if the private files haven't been set up yet. The function file_stream_wrapper_get_instance_by_uri() will return false in this instance.

I have attached another small patch to safely handle this issue.

I found another issue with this code. When downloading a previously uploaded file that has been set to "private://" Drupal will look for any module that implements hook_file_download() to see if any module wants to assign responsibility. As the Feeds module doesn't implement this hook I have created a patch that adds it. Without this hook in place an infinite loop is generated.

The two checks that the hook currently does are:
- Is the file any to do with Feeds?
- Does the user have access to import into this feed.

If no problems are found then the file headers are returned, which prompts Drupal to issue a file download.

It would be great to implement the private file mechanism into some of the tests that are included. I might take it upon myself to do just that :)

Adding keywor for tests as suggested.

Ok. I've finally got around to putting together the test for the private files.

I initially collated everything into a single patch file as the original patches from znerol wouldn't apply to the current version of Feeds. This might need further work but it appears to be behaving nicely at the moment. :)

Thanks Phil! The patch applies, tests pass. The code looks okay. I think that #18 should be commited.

I'm sorry to bump this, but is there any chance of this patch getting applied to the module? I'm happy to do more work on this if anything is missing/broken.

I'm attaching a version of the patch that applies against the 7.x-2.0-alpha5 release (since I needed it there).

This is just for anyone who needs it; the patch to review is still #18. I took a look at that and have a quick review coming up.

Overall, #18 worked perfectly fine for my use case. Testing it a bit more I found a few possible issues, but most of them are small:

1. +      // Verify that the given directory is accessible.
   +      if (!file_prepare_directory($values['source'])) {
   +        form_set_error('feeds[FeedsFileFetcher][source', t('Failed to access the specified directory.'));
   +      }
   

   When a source is manually specified, it's supposed to be possible to specify either a file or directory. This code seems to be assuming that it's always a directory, though. So it seems to break the feature where you can specify a file?

2.    public function sourceFormValidate(&$values) {
   -    $values['source'] = trim($values['source']);
   

   Was removing this line intentional?

3. +    if ($wrapper) {
   +      $description .= l($file->filename, $wrapper->getExternalUrl());
   +    } else {
   +      $description .= 'File wrapper ' . file_uri_scheme($uri) . ' not found.';
   +    }
   

   The last line should probably use t('File wrapper @scheme not found.', array('@scheme' => file_uri_scheme($uri)))? Also, there's a minor code style issue ("else {" should be on its own line).

4. +        watchdog('feeds', 'The upload directory %directory for the feed fetcher !name could not be created or is not accessible. A newly uploaded file could not be saved in this directory as a consequence, and the upload was canceled.', array('%directory' => $feed_dir, '!name' => 'FIXME: figure out name of importer'));
   

   Probably need to resolve (or remove) the FIXME :)

5. +      // Check if choosen url scheme is allowed
   +      if (!in_array(reset(explode(':', $values['source'])), $this->config['allowed_schemes'])) {
   

   I think using reset() inline like that will cause PHP notices in some configurations (since it modifies the input array and therefore needs an actual array to work with). So something that sticks explode(':', $values['source']) into a variable out front would be better.

   Alternatively, wouldn't using file_uri_scheme() here work, and be simpler?

6. Relatively minor, but "chosen" is misspelled as "choosen" in the above example and throughout the patch. This includes at least one user-facing string so it should probably be fixed:

   +    if (!$ok) {
   +      form_set_error('directory', t('The choosen directory does not exist and/or attempts to create it failed.'));
   +    }
   

Beyond those relatively minor issues (well, except the first one, which may not be so minor), the patch looked good to me and seemed to work well, although I'm not much of a Feeds expert myself...

The code is actually correct. Though it may profit from some enlightening comments...

+    if (empty($this->config['direct'])) {

[...] Code for file-upload here [...]

     }
+    else {
+      // Check if choosen url scheme is allowed
+      if (!in_array(reset(explode(':', $values['source'])), $this->config['allowed_schemes'])) {
+        form_set_error('feeds][FeedsFileFetcher][source', t('File needs to reside within the site\'s file directory, its path needs to start with scheme://. Available schemes: @schemes', array('@schemes' => implode(', ', $this->config['allowed_schemes']))));
+      }
+      // Verify that the given directory is accessible.
+      if (!file_prepare_directory($values['source'])) {
+        form_set_error('feeds[FeedsFileFetcher][source', t('Failed to access the specified directory.'));
+      }

The code for direct-mode, i.e. source is a directory on the server is in the else clause.

But that's exactly where the bug is (in direct mode, not file upload).

When direct mode is being used, the text that is displayed on the screen says that it's supposed to work if you type in either an existing directory or an existing file on the server. And before the patch, I'm pretty sure it does. But after the patch, it only seems to work for an existing directory.

Ok, got it. I'm working on a patch.

Voilà.

Thanks for the fix znerol.

I can confirm that both the private files and the direct imports work as expected.

Since @philipnorton42 and @johnv confirmed patch in #26, I'm marking RTBC.

[edit] I didn't try private files, but was able to use a format like:

public://something/myfile.ext

and it used the correct subdirectory (e.g. "something" in example above).

Any comment on if this will be included in future release?

Idea for some stuck on this too and don't want to run patched code, use Apache (or your webserver of choice) to forbid access to files.

Patch #26 works nicely

#26: 1147734-Configurable-upload-directory-for-FeedsFileFetcher-25.patch queued for re-testing.

Awesome patch! Made some changes/improvements.

• Fixed the hook_file_download() implementation.
• Added some #states love to the config form.
• Minor validation fixes.
• Added FeedsFileFetcher::getSchemeOptions() helper funtion.
• Docstring fixes.
• Minor, unrelated doc fixes. While we're here, we might as well.

Should just need a quick review and be good to go.

Nice work! hook_file_download now finally looks sane. I also like the rest of the patch - better validation messages, states, refactoring around getSchemeOptions.

However one thing smells a little bit:

public function configFormValidate(&$values) {

  $values['directory'] = trim($values['directory']);
  $values['allowed_schemes'] = array_filter($values['allowed_schemes']);

  if (!$values['direct'] && !$values['directory']) {
    form_set_error('directory', t('Please specify an upload directory.'));
  }
  elseif (!$values['direct']) {
    $scheme = file_uri_scheme($values['directory']);
    if (!$scheme || !in_array($scheme, $this->getSchemes())) {
      form_set_error('directory', t('Please enter a valid scheme into the directory location.'));

      // Return here so that attempts to create the directory below don't
      // throw warnings.
      return;
    }
  }
  elseif (!$values['direct'] && !file_prepare_directory($values['directory'], FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
    form_set_error('directory', t('The chosen directory does not exist and attempts to create it failed.'));
  }
}

We have three times !$values['direct'] here and it seems to me that the last elseif is dead code.

I think the intention was something like this (patch attached):

public function configFormValidate(&$values) {

  $values['directory'] = trim($values['directory']);
  $values['allowed_schemes'] = array_filter($values['allowed_schemes']);

  if (!$values['direct']) {
    // Ensure that the upload directory field is not empty when not in
    // direct-mode.
    if (!$values['directory']) {
      form_set_error('directory', t('Please specify an upload directory.'));

      // Do not continue validating the directory if none was specified.
      return;
    }

    // Validate URI scheme of upload directory.
    $scheme = file_uri_scheme($values['directory']);
    if (!$scheme || !in_array($scheme, $this->getSchemes())) {
      form_set_error('directory', t('Please enter a valid scheme into the directory location.'));

      // Return here so that attempts to create the directory below don't
      // throw warnings.
      return;
    }

    // Ensure that upload directory exists.
    if (!file_prepare_directory($values['directory'], FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
      form_set_error('directory', t('The chosen directory does not exist and attempts to create it failed.'));
    }
  }
}

Good enough for me.

@zenerol, thanks for sticking with that patch.

7.x http://drupalcode.org/project/feeds.git/commit/38e7898

In latest dev the only available schema is still public://. Will this patch be officially released?

@marktheshark: Please make sure that you set up a valid path for the private file system in admin/config/media/file-system. Also note that this feature is D7 only at the moment.

Thanks for the info. Is it already included in D7 though already or do I need to apply a patch?

@marktheshark: Its included in the dev-version of feeds.

I tried the dev version and the issue still persists - all files are uploaded to the public:// root folder, despite all other settings (files retrieved from private://, URI private, file on content type set to use private).

@gandhiano
Have you set a private file system path at admin/config/media/file-system?
Have you set the "Upload directory" setting of the file fetcher to "private://feeds"?
Or have you mapped to a file field and the files that are being imported don't arive on the private path? That would be a different issue. This issue is about the file you are uploading on the import page.

Yes, I misunderstood - my issue is about the files being imported, there is no problem with reading from a file on a private:// folder (which I'm also doing), so I think this one can be marked as fixed. I will open or look for another issue.