Backup and Migrate module and private file system

I'll look into this. It may be a bug in s3fs, but I think (showing my bias here), that it's more likely to be a bug in Backup and Migrate.

Yup, this is a bug in Backup and Migrate. In several places, Backup and Migrate's code calls the drupal_realpath() function to convert from a stream uri (e.g. private://backup_migrate/manual) to an absolute local filesystem path. That works fine with the usual private:// stream, since it maps to a real location on the server's local filesystem. But since there's no such thing as a realpath for a file in S3, S3fsStreamWrapper->realpath() always returns FALSE. This breaks the function that lists all the available backup files, which is why you get an empty list.

I've attached a quick-and-dirty patch that simply removes the majority of relevant uses of drupal_realpath(). I have no idea what kind of knockon effect this will have, but it does allow files in s3 to be listed in the Saved Backups tab.

I'm going to move this issue to the Backup and Migrate queue, since it really shouldn't be calling drupal_realpath() at all. The docs even specifically discourage its use, for this exact reason.

Moving my comment to correct version...

I throw my hands up in the air at an API function where the first line of the documentation is "Don't use this function"

Seriously though. There are a lot of realpath calls and every one of them is in response to a ticket where somebody's weird setup required it to work. The files directory can be inside the webroot, outside the webroot, it can be a symlink, it can be absolutely defined or relatively or it can be a combination of all these things. Removing the realdir calls will cause regressions that will take years to surface. I'm afraid I'm going to have to favor the 99% use case here where the private directory is local to the server.

If someone can find a simple way of supporting remote streams for the file directory that doesn't break the million other cases that I need to support I'd love to include that, but I'm not sure that should be even undertaken without some sort of automated test coverage.

Maybe you could write a wrapper function around drupal_realpath() that returns the original URI when drupal_realpath() returns FALSE? That shouldn't affect any existing use cases with local filesystems, since drupal_realpath() only returns FALSE if it fails to resolve the real path.

I've run into this issue as well. Here's a patch with a poor man's implementation of @coredumperror's idea, it seems to do the job.

Just ran into this problem and #8 seems to do the job. Performed a manual backup via s3fs, and the file was successfully generated, but no items listed on the "Saved Backups" tab.

After applying the patch, the backup is displayed and able to download it. Thanks for the effort!

Couple of small adjustments... the patch isn't formatted as it should be. Also it's trying to automatically fix extra spaces elsewhere in the module.

Here is an updated version that only includes the relevant code, formatted appropriately. Works as advertised.

One more update. I don't like running the same function on consecutive lines when it has already been evaluated. It's unnecessary processing overhead.

Changed the conditional to capture the result in a variable instead.

Would appreciate a review on the latest patch. Thanks.

I understand wanting to have someone review the update, although the changes I made are *really* minor, and this is a very small patch.

If you want to revert to patch #8 which I already tested and found that it worked correctly, we can do so. However it seems a bit silly given that #14 is just a slightly more efficient version of #8.

@ron_s an interdiff goes a long way to show the minorness of a change.
https://www.drupal.org/documentation/git/interdiff

@joelpittet, I know all about interdiffs. I've created many of them, and so have people in my company. Three comments:

1) Patch #8 no longer applies cleanly to 7.x-3.x-dev, so it's not an option. And I'm not going to dig around trying to find an old version of -dev to create a pseudo-interdiff.

2) A patch is cleaned up that sat idle for 3+ years, and contains literally 8 lines of code. The feedback is more review is necessary and maybe an interdiff should be created. At what point does the Drupal community willingly accept what they receive from those trying to help?

3) I'm all for following standards, and certainly believe in the importance of interdiffs. However, I'm wondering when (if ever) the Drupal community will decide to inject some common sense into its decision making. Things like this drive people away from contributing.

Thanks for the explanation in #16, ron_s. An interdiff on this isn't needed.

FYI I want to test this on a few instances before committing it, just to make sure there aren't any regressions when not using S3.

@ron_s Sorry I didn't mean to offend and probably shouldn't have written anything as I've seen your name in many places and could have guessed you already knew how to make them. I read the

although the changes I made are *really* minor,

and that was the trigger for the reaction comment to keep things moving. Again, my sincere apologies!

No worries @joelpittet. I should have been more explicit and said the changes made were only cleanup. The approach was not modified.

The only change I made was to modify this in the patch...

+     if (drupal_realpath($this->get_location())) {
+       return drupal_realpath($this->get_location());
+     }

into this...

+    if ($realpath = drupal_realpath($this->get_location())) {
+      return $realpath;
+    }

I would hope we can agree the second is more efficient than the first, since drupal_realpath doesn't have to run twice.

Completely agree it's more efficient. I'm not personally a fan of assignments in conditionals, but that debate is happening elsewhere and I wouldn't hold up this issue on that.

RTBC++, thanks @ron_s and @DamienMcKenna

Ah I hadn't see this thread before: https://www.drupal.org/project/coding_standards/issues/2907332

I'd have to say I'm a "0" on the issue. I understand the pros and cons, but not strongly opinionated about it.

Committed. Thanks.