Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.While working on #926700: Upload a theme fails when the temporary directory isn't configured and #929166: Add warning if private files are accessible over the web I noticed a regression in D7 relative to D6.
In D6, the system doesn't let you configure your site without a temporary directory.
In D7, thanks to issues such as #551658: Figure out what to do about new private/public file separation and #346258: use system provided temporary directory (sorry, the cvs blame/issue archeology is a bit hard to follow, and I don't have time to exactly pinpoint where the bug was introduced), we have two nearly identical variables that aren't consistently used: 'file_directory_temp' and 'file_temporary_path'. The end result is that it's possible (in fact, I believe it's the default behavior without going out of your way) to configure your site such that important parts of it don't think you have a temporary directory configured. This breaks all kinds of interesting things, like the Update manager, etc. If you explicitly define a value, it works. But, it appears that in many cases, if you rely on the "default" behavior, you get different default behavior depending on what part of core you're dealing with. :(
I won't pretend that I fully understand our File API (e.g, why are we propagating errors in file.inc via direct calls to drupal_set_message()?), but it sure seems like the split between 'file_directory_temp' and 'file_temporary_path' was unintentional. It's certainly a DX-WTF, and it's leading to subtle bugs in various places.
I propose:
A) We unify these two variables. It looks like 'file_temporary_path' is more consistently used, so let's go with that.
B) We restore the validation we have in D6 if you try to let this variable be empty.
C) We enhance system_requirements() to specifically warn you if your temp directory setting is empty. Right now, the checks for existence and writability just silently ignore the temp directory entirely if "the variable" [sic] isn't defined.
I'll roll a patch for all this, but I wanted to open an issue to get feedback. I've searched high and low and couldn't find an existing bug report specifically about this, and I didn't just want to reopen an old monster issue.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 930122-3.no_file_directory_temp_setting.patch | 3.63 KB | dww |
| #2 | 930122-1.no_file_directory_temp_setting.patch | 2.08 KB | dww |
Comments
Comment #1
mlncn CreditAttribution: mlncn commentedCoder Review module recommends to use the function file_directory_temp() directly, and i don't see any reason not to follow that advice throughout core. In system.install it implies that '$file_temporary_path' could be set in settings.php distinct from '$file_directory_temp', but there is no documentation of such a concept in default.settings.php or anywhere else, and this code would be equally functional (in refraining from triggering the file_directory_temp() function) in using the $file_directory_temp variable.
Also, a reference to checking file_temporary_path on line 2093 of system.module should be removed– it doesn't check it nor does it check file_directory_temp, and it's third in a list that begins "both ..." anyway ;-)
In short, i am in no way a file system expert, but i think all core code should use the function file_directory_temp() everywhere, except system.install, which wants to avoid calling that function during the install phase, and so should use the 'file_directory_temp' variable directly.
Please banish 'file_temporary_path'!
Comment #2
dwwUpon closer inspection, that was surprisingly easy. ;) Careful grep showed that includes/file.inc file_directory_temp() was actually the only place still using the bogus 'file_directory_temp' variable. So, fixing (A) was all of 2 lines of code. However, the way this function handled the variable allowed empty strings or false to count as a value, which isn't okay. So, I changed a !isset() to an empty() to ensure we have some kind of value.
Once (A) was fixed, (B) just magically started working again, thanks to system_check_directory(). The hunk in system.module for that is just a trivial PHPDoc fix.
Once (A) and (B) are fixed, it's basically impossible to end up with an empty value during system_requirements(), so (C) isn't really necessary.
Therefore, this trivial patch should be all we need to make this stuff work sanely again.
Comment #3
dwwOkay, here's a pretty trivial test. If you apply this patch (with the fix and the test), the test passes. If you reverse apply #2 so you're left with just the test, it fails.
Comment #4
dwwRe #1: We can't completely remove a variable here, since we need to let people configure this. It's a bad idea on shared hosting for everyone to use /tmp, for example. So, we need a variable. I don't care that strongly which one we use, so long as we stick with it consistently. Based on my grep findings, it was a much smaller change to standardize on 'file_temporary_path', since that's what the settings form is setup to use, etc, etc. There was just this one place (inside file_directory_temp() itself) that still used the bogus setting.
In IRC, we're debating if it makes sense to just remove the direct calls to variable_get() everywhere in core and always directly call file_directory_temp() (and let it do the variable_get() for you). That seems a bit out of scope here. It's slightly more expensive to do 2 function invocations in the common case, so it's slightly better for performance to do the variable_get() directly and then let the default case figure out a default if the variable isn't defined... But, either way, that's mostly pushing code around for DX. I'm just trying to fix the bug here. ;)
Therefore, I still think #3 is the way to go...
Comment #5
mlncn CreditAttribution: mlncn commentedIn addition to dww's reasons, 'file_temporary_path' fits the pattern used by 'file_public_path' and 'file_private_path' (and of course it is also checked using the function system_check_directory(); i was thrown off by the file_private_path-specific check in there). dww's patch fixes the doxygen syntax of the system_check_directory function and nothing more.
Tested and approved!
For documentation purposes, normal usage of this variable should be:
Or just
Comment #6
rfaySubscribe. Sorry I can't look carefully at this tonight - have to prep for tomorrow. but I'll try to take a look as soon as possible. Just looking quickly at it, I'm sure it's right.
Comment #7
dwwFYI: #931736: No upgrade path for the D6 'file_directory_temp' setting.
Comment #8
dwwNote, the patch/test at #931736-4: No upgrade path for the D6 'file_directory_temp' setting. is going to fail until this fix is in...
Comment #9
chx CreditAttribution: chx commentedWithin the confines of Drupal 7 we can't do anything better. Please after commit reset to 8.x-dev because we immedaitely we want to get rid of the construct
variable_get('file_temporary_path', file_directory_temp());. But this is a valid and necessary fix and we can't do more for now.Comment #10
webchickOk, this fixes a clear oversight that must've been a cross-commit or somehow or other missed in a big find/replace.
Committed #3 to HEAD.
Resetting back to D8 and "active" since a 'file_temporary_path' variable, and a file_directory_temp() function is very weird, but too late to fix in D7.
Comment #11
webchickWell, I thought I did anyway.
Comment #12
dwwLet's clean up the WTF in a new issue so this one can be put to rest.
#931794: 'file_temporary_path' variable and file_directory_temp() function should match
Comment #13
rfayOK, and if I'm not mistaken it's an API change as well (we're invalidating old variables, right?). Please summarize the result.
Thanks for fixing this, @dww
Comment #14
dwwOur community has a very weird definition of what's an "API change"... ;)
However, even if you call renaming this variable is an API change, that's from #551658: Figure out what to do about new private/public file separation or somewhere else (I didn't do a completely thorough cvs archeological dig).
All this issue did was fix a job that was half done elsewhere (or as webchick proposes, introduced by "cross patching")...
Comment #15
rfayThe reason I called it an API change is that a non-core module in development could have used the disappearing variable; after the change it would work completely differently. We've had things like that happen over and over again, as you know. They're hard to debug, and nasty when you debug them and find out that if they'd been announced you would have had a clue.
Comment #16
dww@rfay: I guess my CS background makes me have a stricter definition of what's an "API". But, it's true, global state like variables are part of the "interface" to Drupal. Anyway, yeah, the change from 'file_directory_temp' to 'file_temporary_path' is such a change, but it wasn't introduced here. It should be documented as being introduced via #551658: Figure out what to do about new private/public file separation (I think). If we really want to be specific, we should dig a little deeper at #551658.