Improve Winter\Storm\Filesystem\Zip error reporting

[lex0r]

Package targeted

Storm Library

Description

The wrapper around the standard ZipArchive utility doesn't return a proper error code in case of a failure when extracting the archive. It only returns false which hides the underlying reason for the failure. One can argue whether it's a good approach or not to be verbose. On one hand the intended use cases may not need to know about what exactly happened, on the other hand there may be more clients of the wrapper who don't want to reinvent the wheel or extend the class (although it's not marked as final).

There's a compromise for this particular situation. All the existing clients only check for the response to not be a false one, which can be easily converted to a check like === true without breaking anything and giving other clients an option to see the root cause of the failure.

Will this change be backwards-compatible?

Yes, based on the known usages of the class.

[lex0r]

As a workaround and maybe source of inspiration for a proper fix something like below can be used:

class MyZip extends Zip
{
    public const DEST_EXISTS_OR_UNWRITABLE = 101;

    public const EXTRACT_FAILED = 102;

    /**
     * Extracts an existing ZIP file while return error codes instead of just false in case of error.
     *
     * @param string $source Path to the ZIP file.
     * @param string $destination Path to the destination directory.
     * @param array $options Optional. An array of options. Only one option is currently supported:
     *  `mask`, which defines the permission mask to use when creating the destination folder.
     */
    public static function myExtract(string $source, string $destination, array $options = []): bool|int
    {
        $mask = $options['mask'] ?? 0777;
        if (file_exists($destination) || mkdir($destination, $mask, true)) {
            $zip = new ZipArchive();
            $result = $zip->open($source);
            if ($result === true) {
                $result = $zip->extractTo($destination);
                $zip->close();
                if (!$result) {
                    return self::EXTRACT_FAILED;
                }
            }
            return $result;
        }
        return self::DEST_EXISTS_OR_UNWRITABLE;
    }

    public static function errorAsString(false|int $errorCode): string
    {
        return match ($errorCode) {
            false => 'operation failed',
            MyZip::EXTRACT_FAILED => 'extraction failed',
            MyZip::DEST_EXISTS_OR_UNWRITABLE => 'destination exists or cannot be written to',
            ZipArchive::ER_EXISTS => 'File already exists',
            ZipArchive::ER_INCONS => 'Zip archive inconsistent',
            ZipArchive::ER_INVAL => 'Invalid argument',
            ZipArchive::ER_MEMORY => 'Malloc failure',
            ZipArchive::ER_NOENT => 'No such file',
            ZipArchive::ER_NOZIP => 'Not a zip archive',
            ZipArchive::ER_OPEN => 'Can\'t open file',
            ZipArchive::ER_READ => 'Read error',
            ZipArchive::ER_SEEK => 'Seek error'
        };
    }
}

[bennothommo]

@lex0r thanks for the good proposal.

I seem to recall one of the core plugins does in fact extend the Zip class, so we would have to take backwards-compatibility into account. I do agree however that we should provide some capacity to be verbose if needed.

We probably won't mark any classes final because we promote the idea that people can extend on any of the core functionality however they wish (chiefly, of course, through plugins).

I think your code snippet above is along the idea of where I would approach this too - capturing the error code and making it available in another method so that the original methods still return the same as before. Alternatively, we could opt to log the original error so that there is some record of the failure, although I would prefer someone could have the context available immediately.

We could go along the lines of the json_decode and json_last_error methods in PHP - json_decode returns false if it cannot decode the JSON string, and json_last_error can be used afterwards to get the last error encountered. Any thoughts on this?

[lex0r]

@bennothommo welcome :)

the idea of where I would approach this too - capturing the error code and making it available in another method so that the original methods still return the same as before.

It can return the same while capturing, but I would question why doing that in case all clients are happy with checking for true (and my initial analysis showed they are)?

We could go along the lines of the json_decode and json_last_error methods in PHP - json_decode returns false if it cannot decode the JSON string, and json_last_error can be used afterwards to get the last error encountered. Any thoughts on this?

I don't have any particular preference. The suggested idea doesn't keep the last result which makes it slightly simpler. Also, it doesn't force you to save the error message instantly to avoid loosing it in case another error happens. Not a big deal I agree, but a little more flexible.

[bennothommo]

@lex0r while I agree with you that most (if not all) people just simply check for extract to return false, I cannot garuntee it, therefore our default position is that we have to still be backwards compatible.

Definitely happy for you to add some way of getting the error afterwards, but yeah, we gotta keep the API intact.

[github-actions]

This issue will be closed and archived in 3 days, as there has been no activity in this issue for the last 6 months.
If this issue is still relevant or you would like to see it actioned, please respond within 3 days.
If this issue is critical for your business, please reach out to us at [email protected].