function _private_upload_create_url uses variable_set to momentarily change the file system to 'private' and then back to 'public'. In theory this should be harmless, but we've seen our production site get switched to 'private' filesystem twice now, presumably if something goes wrong in that function. This causes all kinds of problems and is not immediately detected.

This patch uses the variable storage global $conf to temporarily change the setting without actually saving it to the database. This approach seems less dangerous and better performance anyway.

CommentFileSizeAuthor
#5 private.patch1.32 KBJody Lynn
private_upload_file_downloads.patch1.48 KBJody Lynn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented
Title: Private Upload can change private settings back to 'private' » Private Upload can change file settings back to 'private'
jweowu’s picture

jweowu CreditAttribution: jweowu commented

In theory this should be harmless, but we've seen our production site get switched to 'private' filesystem twice now, presumably if something goes wrong in that function.

Using variable_set() is definitely not harmless. It would leave you open to race conditions when multiple requests execute that code simultaneously.

Assuming that the sequence is:
a) store the current setting
b) over-ride with 'private'
c) restore original setting

Then the following could happen:
1. method is 'public'
2. request 1 reads 'public'
3. request 1 writes 'private'
4. request 2 reads 'private'
5. request 2 writes 'private'
6. request 1 restores 'public'
7. request 2 restores 'private'
8. method remains 'private'

Using the global $conf array is correct. It affects the values that variable_get() returns, without writing that value to the database, so it only affects the current request.

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented
Status: Needs review » Reviewed & tested by the community

Thanks for the clearer explanation. Setting to 'reviewed'.

jweowu’s picture

jweowu CreditAttribution: jweowu commented

You might also want to modify the 'HACK' comment accordingly.

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented
FileSize
private.patch1.32 KB
alexharries’s picture

alexharries CreditAttribution: alexharries commented

Seconded - this has broken our site three times today!! :o)

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented
Priority: Normal » Critical
heavy_engineer’s picture

heavy_engineer CreditAttribution: heavy_engineer commented

And there was me thinking i was going mad. This breaks my site every few days or so. Will test the next release.

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented
Version: 6.x-1.0-rc2 » 5.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to the D6 branch.

Needs to be tested for D5 version before being committed to D5.

idmacdonald’s picture

idmacdonald CreditAttribution: idmacdonald commented

This bug makes the 6.x-1.0-rc2 version of the module completely unusable for me. The bug has been fixed in the 6.x-1.x-dev version of the module. I think it would be good to release another stable version of the module soon. I know I prefer to not to run development versions of modules on live sites. And this bug means that the latest 'stable' version of the module is likely to cause major problems for many sites.

Thanks,
-Ian