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

[reimic]

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 ) {
				[...]
			}
		);
	}

[adamziel]

A fix would be useful but I wouldn’t prioritize it. It’s rare to allow failures. Anyway — you suggested, we should log a warning and continue 👍. As for the details, the resource resolver would ideally return something that the step wouldn’t have to treat any differently than a regular file. I’m thinking of a noop-like object, like 1 in multiplications - you can multiply by 1, but it doesn’t change anything.