Comments for "File_CAB"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments

  • Philippe Jausions  [2008-01-29 15:54 UTC]

    __construct() is checking for $this->command when on UNIX but this doesn't allow the user to actually change the path before creating an instance, therefore treating the path as a constant from the user's point of view. To resolve that, either add an argument to the constructor or use static class properties.

    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.

    Some shell escaping of file names and arguments are missing.

    Some of the docblock tags are not in the right order (and some of the @param declarations is jumbled too) See the PEAR CS docblock example for reference.

  • Christian Schmidt  [2008-01-29 22:08 UTC]

    It's great to see CAB support in PEAR.

    You can use the constant OS_WINDOWS to check whether you are on Windows. It is defined in PEAR.php:
    http://pear.php.net/package/PEAR/docs/1.6.2/__filesource/fsource_PEAR__PEAR-1.6.2PEAR.php.html#a45

    Instead of hardcoding C:\windows\system32, you should use one of the environment variables, probably %SYSTEMROOT%.

    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.

    I suggest reporting the file size as an integer, and last_modified as a Unix timestamp (or a DateTime object).

    It is a shame that File_Archive (apparently?) has been abandoned. I hope that we can appoint a new maintainer - at least for the purpose of adding CAB support. It looks as if you can add CAB support without touching too many of the existing files. I think it would be preferable if all supported file archives are accessible through the same API. Even if you prefer to keep File_CAB a separate package, I suggest implementing the same API as File_Archive. Note that the File_Archive CVS repository does in fact have some CAB code in it.