[PEPr] Changes in proposal for File Formats::File_CAB

David Sanders (http://pear.php.net/user/shangxiao) has edited the proposal for File Formats::File_CAB. Change comment: Updated package following comments. Please comment further if the changes do not address what you had originally commented on. > I'd rather see a unique public property for the path to the executable, and > have default private values for Windows and Unix, this way the user only has > to deal with one setting. Since the class is already checking if it runs on > Windows of Unix there's no need to ask the user to set 2 different path > properties. Done. I've used constants for the commands and a public static property for the command. > Some shell escaping of file names and arguments are missing. Done, although I cannot escape "%SystemRoot%...". I thought perhaps using something like realpath() would expand this, but nope. Any suggestions? > Some of the docblock tags are not in the right order Done. > You can use the constant OS_WINDOWS to check whether you are on Windows. I'd rather not build up dependencies and it seems pretty trivial so I've copied this snippet and define the constant OS_WINDOWS if it isn't already defined. > Instead of hardcoding C:\windows\system32, you should use one of the > environment variables, probably %SYSTEMROOT%. Done. Note there is no variable for the complete System32 dir. > If you don't need to require PHP 5.2.1 just because of sys_get_temp_dir(), > you can use System::tmpdir() instead. > You may consider using System::which() in case the cabextract binary is > located elsewhere than /usr/bin. Again, I'd rather not create extra dependencies. Also, I'm not sure I see the point of using which() as this is the same as just using exec("cabextract"). Isn't this a security risk? > I suggest reporting the file size as an integer, and last_modified as a Unix > timestamp (or a DateTime object). Done. I prefer that APIs with dates are standardised by using DateTime. > I suggest implementing the same API as File_Archive. Would be nice, but I'm not sure whether File_Archive can also handle multiple file archives such as cabinets? Please review the proposal: http://pear.php.net/pepr/pepr-proposal-show.php?id=525 -- Sent by PEPr, the automatic proposal system at http://pear.php.net