Not checking files directory recursively before save

The patch looked good, I simplified a bit and separated the part of creation and checking if you can modify it - for creation you don't necessarily need that.

Adjusted the patch a little bit, it doesn't check the files folder itself if it's writable, only its subfolders.

The patch in #2 solved my problem on 6.x-1.x-dev. Thanks!

#2 works for me too, thank you.

Just one tiny bit: since the file is not allways an image, I would prefer $folder instead of $image_folder.
Changed this in the attached patch.

Can you please confirm this problem also affects D7?

If it does then it should be fixed there first.

Thanks!

I got a server with D7 and stage_file_proxy and I think I can test it there.
Hope I get to it tonite.

The D7 version creates the target directory.
It does an file_prepare_directory($target_dir, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS), where $target_dir is the actual save folder.

So there is no need to patch the D7 version.

Much simpler patch.

#8 does not work when there are many levels of directory. This one does.

I tested the patch from #9 and it worked on a file a few levels deep, but there is some functionality change in that directories are created before the curls succeeds. This differs from the functionality of the 7.x branch, which only creates a directory after a successful curl. Here's a version of your patch that only creates directories if the curl is successful, which I think if preferable and expected.

#9 meant a minimal patch for easy review, but #10 works too.

greggles - could we get this committed to the 6.x-1.x-dev branch?

It would be nice to get another review.

I don't maintain the 6.x branch so I don't review/test patches to it.

#9 && #10 works for me

dmitrii - Care to RTBC the issue?

#9 as less resource hungry.

Both #9 and #10 work.

@dmitrii, you say that #9 is less resource hungry, but the difference is incredibly minimal, i.e. an array of just a handful of directory paths. I think the approach in #10 is better because as @samuel.mortenson points out, directories are only created once cURL has checked that the resource actually exists.

Here is an additional patch based on #10 which adds a single check to see if the entire directory path exists before checking/creating each individual subdirectory. This is far better as far as performance, for example: a page contains 5 images that are 5 directories deep means 25 checks, whereas this patch will create the entire directory path for the first image, and each subsequent image only does a single check because the directory exists at that point.

Can we get a couple eyes on this to get it RTBC and committed?

I can confirm that #18 is working well for me. Thanks for the patch!

I applied the patch from #18 successfully on 6.x-1.x-dev. I was having some images not displaying and now they are working.

However, can someone define more clearly the exact issue that this is resolving to test that in particular?

@dooug Without this patch, stage_file_proxy for D6 can only sync files to folders that already exist in sites/[site]/files. So if locally you had no sub-directories in sites/default/files, but you were trying to proxy images from [remote url]/sites/default/files/styles/my-style/example.jpg, that would fail.

A lot of reviews and tests. This is major because the module does not works when parent directory does not exist, which means in a lot of cases.

Thanks for the code and reviews here.

I went with the patch in #18 since it builds on the improvements in #10.

We fixed a denial of service issue in 2013 - https://www.drupal.org/node/2038801 - would be a bummer to reintroduce it ;)