file.inc code review [#7235]

some points i think should be improved in file.inc:

PHP_OS == 'WINNT': this doesn't kick in for Win95(98?), as this has PHP_OS == 'WIN32'. change this test to substr(PHP_OS, 0, 3) == 'WIN'. it might be useful to add a define('OS_WINDOWS', substr(PHP_OS, 0, 3) == 'WIN' ? TRUE : FALSE); (from pear/PEAR.php) for further tests, if needed.
define('FILE_SEPARATOR', PHP_OS == 'WINNT' ? '\\' : '/');: according to the PHP Manual, both slash (/) and backslash (\) are used as path separator character. let's use forward slashes everywhere and get rid of all the os switches, str_replace's etc. in the code
$dest = variable_get('file_directory_temp', (PHP_OS == 'WINNT' ? 'c:\\windows\\temp' : '/tmp'));: on my system, the TMP-dir is D:/tmp. would be nice if this code would use get_cfg_var('upload_tmp_dir') and/or the system default tmp-dir. see this code snippet from the PHP Manual.

Comment	File	Size	Author
#5	file-inc-windows-95-98-issue_0.patch	1.11 KB	Stefan Nagtegaal
#5
#3	file-inc-windows-95-98-issue.patch	1.56 KB	claybutterfly
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

ax CreditAttribution: ax commented 19 April 2004 at 15:19

and a related issue i forgot in my first post: the default "File System Path" ({drupal-dir}/files) should be created and included in drupal-4.4.0.tar.gz. see also Unnecessary step of creating folders for filestore (and maybe also Add directory-properties into README for proper filesystem usage).

Comment #2

ax CreditAttribution: ax commented 20 October 2004 at 21:40

the points from my original post still haven't been implemented and would still make much sense.

Comment #3

claybutterfly CreditAttribution: claybutterfly commented 21 October 2004 at 08:33

Assigned:	Unassigned	» claybutterfly
Priority:	Normal	» Critical

File	Size
file-inc-windows-95-98-issue.patch	1.56 KB

This solves the file issues on windows 95/98..
Tested this and works fine, also not noticed any problems on Windows XP.

File	Size
file-inc-windows-95-98-issue_0.patch	1.11 KB

Comment #6

Steven CreditAttribution: Steven commented 29 October 2004 at 00:01

I think this can be done cleaner with a IS_WINDOWS constant defined once.

Comment #7

Dries CreditAttribution: Dries commented 15 November 2004 at 12:08

I had a look at it, integrated Steven's suggestions, and committed it to HEAD.

Should this be backported to DRUPAL-4-5? That is, does it fix a bug? According to Stefan's comment (comment #3) it does, however, no bug reports are referenced.

Comment #8

Steven CreditAttribution: Steven commented 15 November 2004 at 16:38

I'm not exactly sure what the $regex is supposed to do... it would seem to check for a valid windows drive ('?:\\') but it allows any character for the first character. Also remember that windows PHP allows you to use forward slashes just fine, and that you can go to the root using / as well.