Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling and abstract download classes #52

Merged

Conversation

mcaskill
Copy link
Contributor

@mcaskill mcaskill commented Mar 6, 2023

Checklist

Issue

When an add-on is unsupported or if a download URL can not be retrieved or resolved, an empty string is returned.

This has the unintended consequence of causing Composer to fail much later during the package installation routine resulting in the following error:

Example of the error "Failed to extract junaidbhura/ninja-forms-foobar"

$ composer require junaidbhura/ninja-forms-foobar
./composer.json has been updated
Running composer update junaidbhura/ninja-forms-foobar
Gathering patches from patch file.
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
  - Downloading junaidbhura/ninja-forms-foobar (1.0.0)
Gathering patches from patch file.
Gathering patches for dependencies. This might take a minute.
  - Installing junaidbhura/ninja-forms-foobar (1.0.0): Extracting archive
    Failed to extract junaidbhura/ninja-forms-foobar: (9) '/usr/bin/unzip' -qq '…/vendor/composer/tmp-3e5657bb7bb690e28af71c04f2d6fece' -d '…/vendor/composer/a78af123'

[…/vendor/composer/tmp-3e5657bb7bb690e28af71c04f2d6fece]
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.
unzip:  cannot find zipfile directory in one of …/vendor/composer/tmp-3e5657bb7bb690e28af71c04f2d6fece or
        …/vendor/composer/tmp-3e5657bb7bb690e28af71c04f2d6fece.zip, and cannot find …/vendor/composer/tmp-3e5657bb7bb690e28af71c04f2d6fece.ZIP, period.

    The archive may contain identical file names with different capitalization (which fails on case insensitive filesystems)
    Unzip with unzip command failed, falling back to ZipArchive class
    Install of junaidbhura/ninja-forms-foobar failed

In ZipDownloader.php line 220:

  '…/vendor/composer/tmp-3e5657bb7bb690e28af71c04f2d6fece' is not a zip archive.

To summarize this lengthy error: Composer is attempting to unzip a blank file downloaded from the empty string.

This error has confused many, including myself, as to the real cause. There are numerous issues about it.

Solution

The solution to this is to explicitly throw an exception during the processing of an add-on and the processing of the download URL to abort the package installation process sooner and provide a clear reason for what went wrong.

To simplify the addition of these error messages, I've abstracted common properties and methods into two base classes for greater consistency between plugin downloaders, especially EDD-distributed plugins.

I've chosen UnexpectedValueException since it seems like a good fit for most scenarios based on similar usage in Composer.

There are currently four kinds of error messages thrown:

  1. Package is not supported (Ninja Forms and PublishPress Pro)
  2. Download URL is invalid/missing (EDD and Gravity Forms)
  3. Download version is invalid/missing (EDD only)
  4. Download version and installed version do not match (EDD only)

Example of the error "Could not find a matching package" (1)

$ composer require junaidbhura/ninja-forms-foobar
Info from https://repo.packagist.org: #StandWithUkraine
./composer.json has been updated
Running composer update junaidbhura/ninja-forms-foobar
Gathering patches from patch file.
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 0 removals
  - Locking junaidbhura/ninja-forms-foobar (1.0.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals

In NinjaForms.php line 299:

  Could not find a matching package for junaidbhura/ninja-forms-foobar. Check the package spelling and that the package is supported

Example of the error "Expected a valid download URL" (2)

$ composer require junaidbhura/ninja-forms-foobar
./composer.json has been updated
Running composer update junaidbhura/ninja-forms-foobar
Gathering patches from patch file.
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals

In AbstractEddPlugin.php line 26:

  Expected a valid download URL for package junaidbhura/ninja-forms-foobar

Example of the error "Expected a valid download version number" (3)

$ composer require junaidbhura/ninja-forms-foobar
./composer.json has been updated
Running composer update junaidbhura/ninja-forms-foobar
Gathering patches from patch file.
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals

In AbstractEddPlugin.php line 33:

  Expected a valid download version number for package junaidbhura/ninja-forms-foobar

Example of the error "Expected download version to match installed version" (4)

$ composer require junaidbhura/ninja-forms-foobar
./composer.json has been updated
Running composer update junaidbhura/ninja-forms-foobar
Gathering patches from patch file.
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals

In AbstractEddPlugin.php line 40:

  Expected download version (3.0.0) to match installed version (2.0.0) of package junaidbhura/ninja-forms-foobar

How has this been tested?

I'm testing this on a client project that uses Advanced Custom Fields Pro, ACF Extended Pro, and WPML.

Screenshots

Screen capture of the error "Failed to extract junaidbhura/ninja-forms-foobar"

Screen capture of the error "Could not find a matching package"

Screen capture of the error "Expected a valid download URL"

Screen capture of the error "Expected a valid download version number"

Screen capture of the error "Expected download version to match installed version"

Aggregate common properties and methods for greater consistency between plugin downloaders, especially EDD-distributed plugins.
When returning an empty string, Composer fails later in the installation process and returns a misleading error message:

> Failed to extract junaidbhura/polylang-pro: (9) '/usr/bin/unzip' -qq 'vendor/composer/tmp-XXXXXXXX' -d 'vendor/composer/ZZZZZZ'
>
> [vendor/composer/tmp-XXXXXXXX]
>   End-of-central-directory signature not found.  Either this file is not
>   a zipfile, or it constitutes one disk of a multi-part archive.  In the
>   latter case the central directory and zipfile comment will be found on
>   the last disk(s) of this archive.
> unzip:  cannot find zipfile directory in one of vendor/composer/tmp-XXXXXXXX or
>         vendor/composer/tmp-XXXXXXXX.zip, and cannot find vendor/composer/tmp-XXXXXXXX.ZIP, period.

This error stems from attempting to unzip nothing.

Throwing an exception during the processing of an add-on and of the download URL aborts the installation process sooner and provides a clearer reason for what went wrong.
plugins/AbstractEddPlugin.php Outdated Show resolved Hide resolved
plugins/GravityForms.php Outdated Show resolved Hide resolved
plugins/NinjaForms.php Outdated Show resolved Hide resolved
plugins/PublishPressPro.php Outdated Show resolved Hide resolved
@mcaskill
Copy link
Contributor Author

mcaskill commented Mar 6, 2023

If you prefer, the AbstractEddPlugin could be a trait instead, the concept of the abstract classes could be different if you had something in mind, or each exception could be added to each downloader class.

@junaidbhura
Copy link
Owner

@mcaskill Abstract classes may be easier for potential contributors. Lets merge this! 🚀

@junaidbhura junaidbhura merged commit 6b0284c into junaidbhura:master Mar 6, 2023
@mcaskill mcaskill deleted the feature/improve-error-handling branch March 6, 2023 22:55
@mcaskill mcaskill mentioned this pull request Mar 8, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants