WordPress / WordPress/php-toolkit

HTTP client fails the whole blueprint run if at least one download fails, even when the resource required is for a `continueOnError` step

Open
#107 1 comment 0 reactions 1 assignee View on GitHub

@reimic is already working on this.

Since Jun 8, 2024.

blocker bug HTTP Client
Dominant language
PHP
Stars
61
Forks
21
Avg merge
19h 34m
Merged PRs (30d)
7

Description

Issue:

While writing e2e tests I've noticed the continueOnError functionality does not fully work for steps that download resources.
The client will throw an error that will fail the whole blueprint run if any headers for at least one stream return with a code $code > 399 || $code < 200. This is unexpected as one might think that a CoE step should be able to fail for any reason and do not impact the whole run.

Current behavior is due to this code in streams_send_http_requests:

$headers   = streams_http_response_await_headers( $streams );
		foreach ( array_keys( $headers ) as $k ) {
			$code = $headers[ $k ]['status']['code'];
			if ( $code > 399 || $code < 200 ) {
				throw new Exception( 'Failed to download file ' . $requests[ $k ]->url . ': Server responded with HTTP code ' . $code );
			}
[...]
Example:

For this blueprint:

'{
    "steps":[
        {"step":"installPlugin","pluginZipFile":"https://downloads.wordpress.org/plugin/wordpress-importer.zip"},
	{"step":"installPlugin","pluginZipFile":"https://downloads.wordpress.org/plugin/intentionally-bad-url.zip","continueOnError":true}
    ]
}'

...it can be noticed that the blueprint failed at step 0, while it should not fail at all:

WordPress\Blueprints\Runner\Blueprint\BlueprintRunnerException : Error when executing step installPlugin (number 0 on the list)
[...]

Caused by
Exception: Failed to download file https://downloads.wordpress.org/plugin/intentionally-bad-url.zip: 
Server responded with HTTP code 404
H:\projects\blueprints-library\src\WordPress\AsyncHttp\async_http_streams.php:322
H:\projects\blueprints-library\src\WordPress\AsyncHttp\Client.php:191
Solution:

It seems that doing nothing is better than throwing an exception there.

In my setup the code above is commented out, and multiple InstallPluginSteps are run. An exception for the step with the invalid url is still thrown, but for an issue with activation. (Duh! The resource is not there.) This is then caught in the runner and since that step is continueOnError the exception is suppressed and the blueprint completes. Which is mostly what we want.

But not entirely. Now the issue would be that the InstallPluginStepRunner tries to activate the inexistent plugin and fails. This generates a message that the activation failed, which is only semi-true, because what actually failed is the download.

I imagine the stream context could carry more info on how and why the resource was not procured. This should be checked before activation attempts and a relevant exception should be thrown then. Saying something along the lines of Download failed. Sorry! :)

I have to ponder for a moment how this could be done in detail, however the end result should look more or less like that:

protected function unzipAssetTo( $zipResource, $targetPath ) {
		[...]

		$resource = $this->getResource($zipResource);
		if ( $resource === 'SOMETHING_BAD' ) {
			throw new BlueprintRunnerException("Resource not available because: SOMETHING_BAD");
		}

		$this->getRuntime()->withTemporaryDirectory(
			function ( $tmpPath ) use ( $resource, $targetPath ) {
				[...]
			}
		);
	}

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.