Add optional raw response processing to query context

[mhsdef]

This PR:

Adds QueryContext#process_response so implementors can preprocess before the response hits map_fields.

Why?

Some remote fetches may not be JSON or, even if so, may need manipulation in certain circumstances that only the "client" (implementor) can know.

How To Test

Unit tests passing. Smoke test your favorite remote data blocks in wp-admin and confirm still working.

[mhsdef]

See #25 for the whole experience.

These is how this change is used--context class overrides to shape a non-json response into json:
https://github.com/Automattic/remote-data-blocks/pull/25/files#diff-505e253aa65aba02dd6e26ee91e0a8831236b2f118c08d0597641da93d00a1aeR108

https://github.com/Automattic/remote-data-blocks/pull/25/files#diff-1fb949966a990b11cb49a53b8afa620ac233d8f9d6bd018027a4a6d6115559d4R74

[chriszarate]

We set some early assumptions about supported APIs, one of which was that it transacted in JSON.

I think it's a great idea to provide escape hatches for non-JSON responses and other exceptional attributes. I had intended QueryContext#get_query_runner() to be such an escape hatch. Do you think this approach is better? I worry about bringing complexity into QueryContext, which is designed to be simple and easy to read.

[mhsdef]

We set some early assumptions about supported APIs, one of which was that it transacted in JSON.

And yet. 🤪

I think it's fine if that's what we say on the tin, but, besides the need I have for otherwise right here... I could see situations with others that might want to do the same.

For example, maybe I'd like to read a static CSV file directly from somewhere.

Do you think this approach is better?

I am not saying it's the only way or best way. I tried to thread the needle of a change that stayed true to your vision here. FWIW, it gets the job done and, I think, semantically makes decent sense.

I worry about bringing complexity into QueryContext, which is designed to be simple and easy to read.

Welp, the upside, now QueryRunner would be simple and easy to read. 😆

[mhsdef]

I had intended QueryContext#get_query_runner() to be such an escape hatch.

Yeah, we could do that. I'm glad that customization exists. I considered that but, as a "customer" implementing, I didn't like that I had to re-implement the whole thing just to manipulate the response. I wanted the world-class WP developer request handling 💪 done for me. And I didn't want to copy/paste.

[chriszarate]

For example, maybe I'd like to read a static CSV file directly from somewhere.

+1

Welp, the upside, now QueryRunner would be simple and easy to read. 😆

I like when complexity only presents itself when you need to confront it. Ideally, most extenders of QueryContext (who primarily use JSON APIs) will not need to investigate QueryRunner or follow the complex logic of how it parses JSON.

That's one complication with moving map_fields to QueryContext. The other is that QueryRunner provides the shape and structure of data expected by the front-end block editor code. Delegating map_fields to the customer is riskier.

I think you've correctly pointed out that the abstraction isn't quite right. For non-JSON response formats like HTML and CSV, I'm wondering if there's a way to delegate just the basic response parsing and leave the "field mapping" as-is.

[mhsdef]

I'm wondering if there's a way to delegate just the basic response parsing and leave the "field mapping" as-is.

I considered that. There is. It requires the context to have fairly intimate knowledge about how map_fields works. Or we need to widen what map_fields can handle as input.

The former didn't feel good because of the mental overhead and re-encoding to a JSON string and then extra decodes in map_fields. The latter might work, although not entirely trivial surgery on the function, so it fails your as-is test.

[mhsdef]

I went ahead and made the smaller incision since I think you feel more strongly about this than I do.

I don't love the need to json_encode just to make map_fields happy and (possibly) json_decode 2x if the raw response was already json. But I do like that the override is simpler:

	public function process_response( string $html_response_data, array $input_variables ): string {
		return json_encode(
			[
				'content'   => $html_response_data,
				'file_path' => $input_variables['file_path'],
				'sha'       => $input_variables['sha'],
				'size'      => $input_variables['size'],
				'url'       => $input_variables['url'],
			]
		) ?? '';
	}

[mhsdef]

I confirmed, as expected, core escaping happens further on in execution--somewhere closer to output.

[chriszarate]

escapin late :)

[chriszarate]

I don't love the need to json_encode just to make map_fields happy and (possibly) json_decode 2x if the raw response was already json

I pushed up some type annotations, comments, and tests to show that this isn't necessary. JsonObject accepts a JSON string, a PHP associative array, or standard object.

[chriszarate]

What is the use case for this is_collection() method override? Maybe the response needs to be inspected? In that case, it seems like it should get passed the $response_data? Or removed

[chriszarate]

Removing arg $is_collection means that is_collection() gets called twice. Can we get it back to one call?

[chriszarate]

Approved, but would like to see the is_collection() Qs resolved somehow

[mhsdef]

JsonObject accepts a JSON string, a PHP associative array, or standard object.

Ha, and so it does. TIL.

Yeah, that makes me happier.