Admin interface

The following patch adds a settings page for the three settings this module uses. It also adds a very small documentation header to each of the functions in the module, and fixes a typo in the INSTALL.txt file.

The patch in #1 above is for D6. Here's a patch for D7.

Tested the 7.x patch - looks good.

Just a reminder that if this gets committed, the issue should be set back to "needs review" and "6.x-1.x-dev" instead of fixed.

I've added a form item for the variable I added in http://drupal.org/node/1444208#comment-5616210

Here is another patch that combines the both patches from #4 and #2 into one patch. You can't use patch #4 unless you apply #2, so they might as well be put together. ;-)

Can we get this committed? I use this patch every time I install the module, which is about weekly now (yes, I have that many dev sites).

No issues whatsoever with the D6 patch.

The D7 patch needs re-rolling.

I don't see why this is a big deal, but I'm open to the idea that it's worth including.

I only care about 7.x, though, so it would be nice to get a reroll for that for me to apply it.

What do folks think about #1871716: add hook_enable message with minimal instructions to get running as a lighter weight solution?

hook_enable() only appears once ;-(
IMO renaming INSTALL.txt to README.txt would be sufficient. The README.txt will appear magically on the module page in the last column. (perhaps after enabling advanced_help module).
in the info-file, you can also add a 'configuration = URL' line. Normally this goes to an admin page, but you might link to the README.txt.

I don't think we could link to README.txt without making a callback in the module that serves it up regardless of where it is. The info file is static, but the location installation of the module might vary wildly on different sites.

The patch in #5 no longer applies, but once it does I'll be happy to commit it. I was reminded today that there are a lot of Drupal users in the world and making them all resort to settings.php changes probably feels a bit barbaric to many of them.

Here is a patch for D6

Thanks @rlnorthcutt! I don't use d6 so I'll need someone else to review it for that version.

I tested the patch and I really like this. I also was not able to get the module to work by putting the raw code suggested in INSTALL.txt in my settings.php file but once I had the admin UI, it "just worked", I have no idea why. This is a really nice feature, thanks.

The patch in #12 has a bunch of unrelated changes as well. It would be good to know if those are backports of other issues or what.

It also no longer applies after #2022179-4: SA-CONTRIB-2013-056 - Remote file fetching doesn't work if files directory doesn't exist and creates the directory first.

I'd really like to commit this to 7.x-1.x first and backport, so updating version.

Here's a patch with just the interface stuff, for D7, based on 7.x-1.x.

I have a slightly different take on why we can use an admin interface. For starters, we run a 4 tier deployment system: development (my laptop), testing, staging, and production. I only have access to my development environment, so stage file proxy is FANTASTIC. However, that being said, since I don't have access to testing and staging, I can't get to the settings.php file. Therefore a GUI admin interface would be perfect.

Also, within our group, we have a number of designers/front-end developers (using the term developer loosely) and the thought of a command line interface haunts them and they prefer a GUI interface (go figure). That being said, for some reason, they absolutely require developing a theme with real content, including images, so stage file proxy is FANTASTIC. But the lack of an admin interface bums them out.

Have I mention how fantastic stage file proxy is???

Anyway, I also have a patch for an admin interface, however, my patch is slightly different:

• it contains all variables available to the settings.php file
• I added a hook_uninstall to delete all the variables set by the admin interface
• I validate the variables in order to make sure they are not set to the default values, otherwise we should rely on the modules to supply the default values
• add a new permission called 'administer stage_file_proxy settings'

I'm open for suggestions, but I highly recommend using my patch, since it's been tested within my group for a while now and it cleans up nicely on uninstall

I like all of that except:

+  // Check if fields contain default values, if the default value is set
+  // then we need to remove the system variables and use default values from the module

I haven't seen a form that works like this and I'm not sure I understand the motivation for doing it. Do you know of other forms that do this?

@greggles, the motivation is that if you set the origin, but NOT the origin_dir, I don't want the system variable table to contain a blank origin_dir value. So if the values are set to the default (or blank), then it removes the system variables. I'm not sure if any other module is doing this, I just tried it myself and it seems to work.

I don't want the system variable table to contain a blank origin_dir value

Can you clarify why that's a problem? I looked at the code and don't see (yet) how it's a problem.

Without my submit function blowing away the system variable if origin_dir is blank, then the following line chokes:

$remote_file_dir = trim(variable_get('stage_file_proxy_origin_dir', $file_dir),'/');

The variable_get function will return a blank string. This was the only solution I could come up with, without touching your original code. Is it a perfect solution? Probably not, but I'm open for suggestions.

So let's say hypothetically that someone has / as their remote files directory. That is, they let the webserver put the files at the root of the site. It seems like this admin interface wouldn't be able to handle that, right?

How about changing the admin form from this

'#default_value' => variable_get('stage_file_proxy_origin_dir', ''),

to this:

'#default_value' => variable_get('stage_file_proxy_origin_dir', variable_get('file_public_path', conf_path() . '/files')),

Wouldn't that work out right?

Excellent point...we can't assume the origin server is a Drupal server (or for some reason they upload files to their Drupal's root folder) and we don't want to isolate those use cases. Let me reroll my patch.

Pardon my intrusion, but isn't the whole point of this module to enable dev and staging sites to pull files from the production site, which by necessity would have the same structure and platform (i.e. Drupal)?

I suppose the file paths could be aliased to something other than 'sites/[site]/files', but I don't think we shouldn't assume the origin site is a Drupal site.

Just my $0.015.

Re-rolled patch from #18.

-set default for origin directory to local site's file folder
-Removed my custom submit callback and reverted back to system_settings_form, since we no longer need to delete system variables
-added one more validate for SSL version

StephenBrown - Your comment is very welcome. That is the original intent of the module, but it's apparent to me that people are using it for other purposes as well. If we can support a broad set of needs and reduce the number of lines of code in the process...seems like a win-win to me.

Anyone able to review patch in #26?

@StephenBrown, I agree 100%, but I don't want to isolate the less than .00001% of folks who might be using an external CDN for their images. Plus @greggles default value for origin_dir is a much better solution than mine. But you're right.....most folks will be pulling images from a Drupal server.

Ah, I hadn't considered a CDN (but in that case, couldn't the images just stay on the CDN? More niggling.)

And yes, the default is good for the 99.999% of cases. Carry on. :-)

re #29: One benefit of stage_file_proxy is that you can configure it to download the images locally for use when offline. So, keeping them on the CDN doesn't work for a decent percent of the module's users.

I tested the patch in comment #26. First, I patched Stage File Proxy 1.4 on my local environment without deactivating the module, and it worked fine. Then I deactivated the module, uninstalled it, and reactivated Stage File Proxy, and the patch worked fine.

Love it. #26 works great for me.

This works great. Thanks!

Re-rolling patch from #26 to latest version of Stage File Proxy, please test

Tested, and worked like a charm :-)

I had two followups: the one in comment #37 is for code style issues I identified using http://drupal.org/project/coder

The second I snuck into #36 to make the permision "restrict access" => true so that the permission won't be granted to people without really really trusting them.

Forgot to change the status. I don't really maintain D6 so if someone wants to backport and get a review then I could commit that (or maybe make those people maintainers of the d6 version).