Rather than using '/' to separate directories when we're building paths, we should use the PATH_SEPARATOR constant. The change fixes some bugs I've encountered on Windows using file uploads.

According to the docs on php.net it was introduced in PHP 4.3.0-RC2. The installation notes list 4.3.3 as our minimum requirement in so compatibility shouldn't be a problem.

Please review the attached patch. I've tried to make sure that the changes only affect files, not URLs.

CommentFileSizeAuthor
#1 file.inc.USING_DIRECTORY_SEPARATOR_0.patch3.45 KBdrewish
file.inc.USING_DIRECTORY_SEPARATOR.patch3 KBdrewish

Comments

drewish’s picture

drewish commented
StatusFileSize
new file.inc.USING_DIRECTORY_SEPARATOR_0.patch3.45 KB

that last patch had a bug in it. it was setting \s on urls.

drewish’s picture

drewish commented
Title: file.inc should use PATH_SEPARATOR when building paths » file.inc should use DIRECTORY_SEPARATOR when building paths

whoops, wrong constant in the title

morbus iff’s picture

morbus iff
they/them
commented

I'm a little ... ... well, I can't say with any certainty if this is a good idea or not. Neither constant seems to have very decent documentation and my tests, on OS X and webchick's win32 box, shows that carry the same value (I originally thought that PATH may be related to a URL, but it's not - perhaps they're thinking UNC paths?). They return what I expect, certainly ("/" on *nix and "\" on Win32), but I'd like to hear more about the bugs you're facing with the current code.

morbus iff’s picture

morbus iff
they/them
commented

Based on some discussions in freenode's #php, "/" is supposed to be handled transparently and crossplatform-like internally to PHP. So, instead of "c:\\winnt\\tmp", for example, it'd be "c:/winnt/tmp". They suggest there isn't a need for DIRECTORY SEPARATOR at all. I've been unable, however, to find official documentation that suggests the same.

morbus iff’s picture

morbus iff
they/them
commented

According to dirname (and other directory/path related functions): On Windows, both slash (/) and backslash (\) are used as directory separator character. In other environments, it is the forward slash (/). Which would suggest that the _SEPARATOR stuff isn't needed in Drupal, and we should change my hardcoded "c:\\" stuff to "c:/" instead.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work
ax’s picture

ax commented
Status: Needs work » Closed (won't fix)

we'll definitely want to use / instead of DIRECTORY_SEPARATOR. see also http://drupal.org/node/7235. marking WON'T FIX.