Improve error handling and logic #58

mcaskill · 2023-03-08T23:48:06Z

Checklist

I've read the Contributing page.
Issues: Class Wpml does not comply with psr-4 autoloading standard #54, PSR-4 Incompatibility #55, Poylang Pro Installation fails #56, Drop support for Composer 1? #57
My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

Draft 2 — 2023-03-15

Description

Follow-up to #52 and alternative to the initial draft (see below).

Primary focus is improving error handling by validating the response from cURL requests, validating the decoded/unserialized response bodies, and simplifying the logic between plugins.

How has this been tested?

I'm testing this on client projects that use ACF Pro, ACF Extended Pro, PublishPress Pro, Gravity Forms, Polylang Pro, and WPML.

Screenshots

Types of changes

Plugins:
- ACF Pro, Gravity Forms, WPML: Change URL formatting to use http_build_query() to improve readability and reduce risk of mistakes.
- ACF Extended Pro, Gravity Forms, Polylang Pro: Change HTTP request method to use GET instead of POST to better match the verb of the request and for consistency between the other downloader. Both Gravity Forms' API and Easy Digital Downloads' API support GET requests.
- Ninja Forms, PublishPress Pro, WPML: Throw InvalidArgumentException instead of UnexpectedValueException when dealing with an unsupported package.
- EDD Plugins, Gravity Forms: Throw UnexpectedValueException if the decoded/unserialized API response is not an array.
- EDD Plugins: Replaced getDownloadUrl() with a new protected method getDownloadUrlFromApi() that only handles the request for the download URL response object.
AbstractEddPlugin:
- Replaced extractDownloadUrl() with getDownloadUrl() that retrieves the response to parse from the plugin's getDownloadUrlFromApi().
- Wrapped requests with Http in a try/catch to intercept cURL exceptions and wrap them in a plugin-aware exception.
Http:
- Added protected method request() to aggregate the execution of the request, handling of the response, and closing of the cURL handler.
- Added cURL option CURLOPT_FAILONERROR to fail if the response is >= 400.
- Throw RuntimeException if the request failed.
- Compile URL in GET request earlier to organize cURL options similarly to POST request.
- Use booleans intead of integers for cURL options.

Draft 1 — 2023-03-07

Description

Follow-up to #52.

Fixed the PSR-4 file name for Wpml class, for the sake of testing.

Improve checks on HTTP response. Check if status code is OK (200) and if the body is an array, throw exceptions if not with an excerpt of the response body for aid with debugging.

Expanding upon #47, improved Gravity Forms to check if download version matches the package's version with support for either available download: the "main version" (download_url) or the "latest version" (download_url_latest). For example:

version: 2.7.2 (PARADIGM.MAJOR.MINOR)
version_latest: 2.7.2.1 (PARADIGM.MAJOR.MINOR.PATCH)

As of 2023-03-08, this code sort of works but is not completely tested/vetted, nor is it the best design, and some of it should be split into their own pull requests.

If we drop support for Composer 1, a lot of this proposed code can be simplified to take advantage of Composer 2's utilities.

I'm open to comments, suggestions, and contributions.

How has this been tested?

I'm testing this on client projects that use ACF Pro, ~~ACF Extended Pro~~, ~~PublishPress Pro~~, Gravity Forms, Polylang Pro, and WPML.

Types of changes

Change URL formatting (for ACF, Gravity Forms, WPML) to use http_build_query() to improve readability and reduce risk of mistakes.
Change HTTP request method (for ACF Extended, Gravity Forms, Polylang) to use GET instead of POST to better match the verb of the request and for consistency between the other downloader. Both Gravity Forms' API and Easy Digital Downloads' API support GET requests.
Refactored Http class to store last response body and headers, allow parsing of body through callback, and finding of status code and message (based on Composer 2's Response, RemoteFileSystem, and HttpDownloader).
Use Composer's Semver to check Gravity Forms versions and throw an exception if we download the incorrect version.

junaidbhura · 2023-03-12T21:41:28Z

Looks good @mcaskill are you still working on this?

mcaskill · 2023-03-13T13:30:33Z

Thanks. Unfortunately, at this time, I won't be able to continue/finish work on this feature. If you or someone else wants to take over, I can add them to the branch.

~~Similarly, could you fix the WPML class (WPML.php → Wpml.php) to resolve the open issue?~~

Use `http_build_query()` to improve readability and reduce risk of errors.

Use GET instead of POST to better match the verb to the request and for consistency between downloaders.

Changed: - Throw `InvalidArgumentException` instead of `UnexpectedValueException` when dealing with an unsupported package in Ninja Forms, PublishPress Pro, and WPML. - Throw `UnexpectedValueException` if the decoded/unserialized API response is not an array. - Improved messages to clarify errors from API. - Fixed and improved PHPDoc tags to document exceptions.

Changed: - Use booleans intead of integers for cURL options. - Compile URL in GET request earlier to organize cURL options similarly to POST request.

Changed: - Added protected method `request()` to aggregate the execution of the request, handling of the response, and closing of the cURL handler. - Added cURL option `CURLOPT_FAILONERROR` to fail if the response is >= 400. - Throw `RuntimeException ` if the request failed. - Fixed and improved PHPDoc tags.

Changed: - Replaced `getDownloadUrl()` in EDD plugins with a new protected method `getDownloadUrlFromApi()` that only handles the request for the download URL response object. - Replaced `extractDownloadUrl()` with `getDownloadUrl()` in `AbstractEddPlugin` that retrieves the response to parse from the plugin's `getDownloadUrlFromApi()`. - Wrapped requests with `Http` in a try/catch to intercept cURL exceptions and wrap them in a plugin-aware exception.

mcaskill · 2023-03-16T00:46:10Z

I've updated the pull request title and description to reflect the latest state of proposed changes.

The initially proposed changes for Gravity Forms will be resubmitted as its own pull request once we've figured out this request.

A URL is necessary for requests.

Changed: - Removed extraneous property descriptions. - Rephrased description of `$slug` property to be more specific. - Added missing `$slug` method parameter.

Amends cee1cf5 Accidentally reverted back from GET to POST.

Decoupled API URL and API URL query parameters from its HTTP query building and URL concatenation, and HTTP request call. This allows the base URL and query parameters to be more readable from their final concatenated URL or HTTP request call.

This ensures compatibility with the classes' `$version` property being potentially empty, presuming a request for the latest version.

To reduce repetition, and risk of mistakes, when formatting the plugin's Composer package name. The package name is used in the messaging of most exceptions thrown. Storing vendor name as a namespace

Changed: - Moved shared cURL options to `request()` method. - Catch any exception from `Http` requests.

Separated JSON decoding error handling from HTTP request/response error handling. By using `json_last_error()`, we can provide a more precise error for end users as opposed to just checking if its an array. Output a truncated excerpt of the response body if JSON decoding fails to give a hint of what the issue might be.

mcaskill · 2023-06-09T20:43:15Z

I found some time to do some further testing and improvements.

I think it should be functional enough.

If people can test, that would be appreciated.

Separated unserialization error handling from HTTP request/response error handling. Wrapped unserialization in try/catch to intercept any throwables from unexpected objects in their unserialization handlers. Gravity Forms is supposed to return an associative array but could be compromised. Output a truncated excerpt of the response body if unserialization fails to give a hint of what the issue might be. Ideally, add an error handler to catch the PHP notice that is raised by `unserialize()`.

From 'mcaskill/feature/refactor-http-class': Improve error handling and logic

junaidbhura approved these changes Mar 12, 2023

View reviewed changes

mcaskill force-pushed the feature/refactor-http-class branch from 5b3a2d9 to 05d1612 Compare March 15, 2023 15:12

mcaskill added 6 commits March 15, 2023 20:03

Change URL formatting for ACF, GF, WPML

23e7a41

Use `http_build_query()` to improve readability and reduce risk of errors.

Change HTTP request method for ACF-E, GF, PLL

2810340

Use GET instead of POST to better match the verb to the request and for consistency between downloaders.

Clean-up Http class

0b1c8af

Changed: - Use booleans intead of integers for cURL options. - Compile URL in GET request earlier to organize cURL options similarly to POST request.

mcaskill force-pushed the feature/refactor-http-class branch from 05d1612 to cee1cf5 Compare March 16, 2023 00:41

mcaskill changed the title ~~Improve HTTP response and error handling~~ Improve error handling and improve logic Mar 16, 2023

mcaskill marked this pull request as ready for review March 16, 2023 00:42

mcaskill requested a review from junaidbhura March 16, 2023 00:42

This was referenced Mar 16, 2023

Fail if we download the incorrect version of Gravity Forms #60

Closed

Add EditorConfig and PHPStan #61

Closed

mcaskill mentioned this pull request May 11, 2023

WP All Import download fails. #64

Open

mcaskill added 8 commits June 9, 2023 11:17

Remove default value for $url arguments in Http class

24eb239

A URL is necessary for requests.

Update block comments in AbstractPlugin

1e8f528

Changed: - Removed extraneous property descriptions. - Rephrased description of `$slug` property to be more specific. - Added missing `$slug` method parameter.

Fix HTTP request method for ACF-Extended

378ade7

Amends cee1cf5 Accidentally reverted back from GET to POST.

Decouple API URL and query parameters

efdb917

Decoupled API URL and API URL query parameters from its HTTP query building and URL concatenation, and HTTP request call. This allows the base URL and query parameters to be more readable from their final concatenated URL or HTTP request call.

Decouple version from API URL query parameters and version check

e667513

This ensures compatibility with the classes' `$version` property being potentially empty, presuming a request for the latest version.

Add getPackageName() method to AbstractPlugin

f4ad7ec

To reduce repetition, and risk of mistakes, when formatting the plugin's Composer package name. The package name is used in the messaging of most exceptions thrown. Storing vendor name as a namespace

Improve Http class

fa45ae8

Changed: - Moved shared cURL options to `request()` method. - Catch any exception from `Http` requests.

mcaskill force-pushed the feature/refactor-http-class branch from c736634 to 03771dd Compare June 9, 2023 20:45

mcaskill force-pushed the feature/refactor-http-class branch from 03771dd to ab73a24 Compare June 9, 2023 20:46

mcaskill changed the title ~~Improve error handling and improve logic~~ Improve error handling and logic Jun 9, 2023

This was referenced Jul 11, 2024

Improve error handling and logic mcaskill/composer-wp-pro-plugins#2

Merged

Fail if we download the incorrect version of Gravity Forms mcaskill/composer-wp-pro-plugins#3

Merged

Add EditorConfig and PHPStan mcaskill/composer-wp-pro-plugins#4

Merged

mcaskill closed this Jul 12, 2024

mcaskill deleted the feature/refactor-http-class branch July 12, 2024 22:31

mcaskill added a commit to wp-jazz/composer-wp-pro-plugins that referenced this pull request Jul 13, 2024

Merge pull request junaidbhura/composer-wp-pro-plugins#58

67ac927

From 'mcaskill/feature/refactor-http-class': Improve error handling and logic

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling and logic #58

Improve error handling and logic #58

mcaskill commented Mar 8, 2023 •

edited

Loading

junaidbhura commented Mar 12, 2023

mcaskill commented Mar 13, 2023 •

edited

Loading

mcaskill commented Mar 16, 2023 •

edited

Loading

mcaskill commented Jun 9, 2023

Improve error handling and logic #58

Improve error handling and logic #58

Conversation

mcaskill commented Mar 8, 2023 • edited Loading

Checklist

Draft 2 — 2023-03-15

Description

How has this been tested?

Screenshots

Types of changes

Draft 1 — 2023-03-07

Description

How has this been tested?

Types of changes

junaidbhura commented Mar 12, 2023

mcaskill commented Mar 13, 2023 • edited Loading

mcaskill commented Mar 16, 2023 • edited Loading

mcaskill commented Jun 9, 2023

mcaskill commented Mar 8, 2023 •

edited

Loading

mcaskill commented Mar 13, 2023 •

edited

Loading

mcaskill commented Mar 16, 2023 •

edited

Loading